diff --git a/adserver/auth/admin.py b/adserver/auth/admin.py index 928742e0..e184013c 100644 --- a/adserver/auth/admin.py +++ b/adserver/auth/admin.py @@ -6,6 +6,22 @@ from simple_history.admin import SimpleHistoryAdmin from .models import User +from .models import UserAdvertiserMember +from .models import UserPublisherMember + + +class UserAdvertiserInline(admin.TabularInline): + """For inlining the user-advertiser relationship.""" + + model = UserAdvertiserMember + raw_id_fields = ("advertiser",) + + +class UserPublisherInline(admin.TabularInline): + """For inlining the user-publisher relationship.""" + + model = UserPublisherMember + raw_id_fields = ("publisher",) @admin.register(User) @@ -19,8 +35,6 @@ class UserAdmin(SimpleHistoryAdmin): _("Ad server details"), { "fields": ( - "advertisers", - "publishers", "flight_notifications", "notify_on_completed_flights", # DEPRECATED ) @@ -43,6 +57,7 @@ class UserAdmin(SimpleHistoryAdmin): {"fields": ("last_login", "updated_date", "created_date")}, ), ) + inlines = (UserAdvertiserInline, UserPublisherInline) list_display = ( "email", "name", diff --git a/adserver/auth/migrations/0009_user_advertiser_publisher_roles.py b/adserver/auth/migrations/0009_user_advertiser_publisher_roles.py new file mode 100644 index 00000000..5f84afe6 --- /dev/null +++ b/adserver/auth/migrations/0009_user_advertiser_publisher_roles.py @@ -0,0 +1,70 @@ +""" +This migration was autocreated but has been customized. + +See: https://docs.djangoproject.com/en/5.0/howto/writing-migrations/#changing-a-manytomanyfield-to-use-a-through-model +""" + +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('adserver', '0098_rotation_aggregation'), + ('adserver_auth', '0008_data_flight_notifications'), + ] + + operations = [ + migrations.SeparateDatabaseAndState( + state_operations=[ + migrations.CreateModel( + name='UserAdvertiserMember', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + # We add this in a separate operation below + # ('role', models.CharField(choices=[('Admin', 'Admin'), ('Manager', 'Manager'), ('Reporter', 'Reporter')], default='Admin', max_length=100)), + ('advertiser', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='adserver.advertiser')), + ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), + ], + options={ + 'db_table': 'adserver_auth_user_advertisers', + }, + ), + migrations.AlterField( + model_name='user', + name='advertisers', + field=models.ManyToManyField(blank=True, through='adserver_auth.UserAdvertiserMember', to='adserver.advertiser'), + ), + migrations.CreateModel( + name='UserPublisherMember', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + # We add this in a separate operation below + # ('role', models.CharField(choices=[('Admin', 'Admin'), ('Manager', 'Manager'), ('Reporter', 'Reporter')], default='Admin', max_length=100)), + ('publisher', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='adserver.publisher')), + ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), + ], + options={ + 'db_table': 'adserver_auth_user_publishers', + }, + ), + migrations.AlterField( + model_name='user', + name='publishers', + field=models.ManyToManyField(blank=True, through='adserver_auth.UserPublisherMember', to='adserver.publisher'), + ), + ], + ), + migrations.AddField( + model_name="useradvertisermember", + name="role", + field=models.CharField(choices=[('Admin', 'Admin'), ('Manager', 'Manager'), ('Reporter', 'Reporter')], default='Admin', max_length=100), + ), + migrations.AddField( + model_name="userpublishermember", + name="role", + field=models.CharField(choices=[('Admin', 'Admin'), ('Manager', 'Manager'), ('Reporter', 'Reporter')], default='Admin', max_length=100), + ), + ] diff --git a/adserver/auth/models.py b/adserver/auth/models.py index 9d2d7889..0fe9ec92 100644 --- a/adserver/auth/models.py +++ b/adserver/auth/models.py @@ -79,8 +79,12 @@ class User(AbstractBaseUser, PermissionsMixin): created_date = models.DateTimeField(_("create date"), auto_now_add=True) # A user may have access to zero or more advertisers or publishers - advertisers = models.ManyToManyField(Advertiser, blank=True) - publishers = models.ManyToManyField(Publisher, blank=True) + advertisers = models.ManyToManyField( + Advertiser, blank=True, through="UserAdvertiserMember" + ) + publishers = models.ManyToManyField( + Publisher, blank=True, through="UserPublisherMember" + ) # Notifications flight_notifications = models.BooleanField( @@ -115,6 +119,54 @@ def get_full_name(self): def get_short_name(self): return self.get_full_name() + def get_advertiser_role(self, advertiser): + """ + Returns the users role in this advertiser or None if the user has no permissions. + + Staff status is not taken into account. Caches the result on the user so future calls + don't involve a DB lookup. + """ + if not hasattr(self, "_advertiser_roles"): + self._advertiser_roles = {} + + if advertiser.pk in self._advertiser_roles: + return self._advertiser_roles[advertiser.pk] + + membership = self.useradvertisermember_set.filter( + advertiser=advertiser, + ).first() + + role = None + if membership: + role = membership.role + + self._advertiser_roles[advertiser.pk] = role + return role + + def get_publisher_role(self, publisher): + """ + Returns the users role in this publisher or None if the user has no permissions. + + Staff status is not taken into account. Caches the result on the user so future calls + don't involve a DB lookup. + """ + if not hasattr(self, "_publisher_roles"): + self._publisher_roles = {} + + if publisher.pk in self._publisher_roles: + return self._publisher_roles[publisher.pk] + + membership = self.userpublishermember_set.filter( + publisher=publisher, + ).first() + + role = None + if membership: + role = membership.role + + self._publisher_roles[publisher.pk] = role + return role + def get_password_reset_url(self): temp_key = default_token_generator.make_token(self) path = reverse( @@ -131,6 +183,36 @@ def get_password_reset_url(self): scheme=scheme, domain=domain, path=path ) + def has_advertiser_permission(self, advertiser): + role = self.get_advertiser_role(advertiser) + return role is not None + + def has_advertiser_manager_permission(self, advertiser): + role = self.get_advertiser_role(advertiser) + return role in ( + UserAdvertiserMember.ROLE_ADMIN, + UserAdvertiserMember.ROLE_MANAGER, + ) + + def has_advertiser_admin_permission(self, advertiser): + role = self.get_advertiser_role(advertiser) + return role == UserAdvertiserMember.ROLE_ADMIN + + def has_publisher_permission(self, publisher): + role = self.get_publisher_role(publisher) + return role is not None + + def has_publisher_manager_permission(self, publisher): + role = self.get_publisher_role(publisher) + return role in ( + UserPublisherMember.ROLE_ADMIN, + UserPublisherMember.ROLE_MANAGER, + ) + + def has_publisher_admin_permission(self, publisher): + role = self.get_publisher_role(publisher) + return role == UserPublisherMember.ROLE_ADMIN + def invite_user(self): site = get_current_site(request=None) @@ -146,3 +228,55 @@ def invite_user(self): [self.email], ) return True + + +class UserAdvertiserMember(models.Model): + """User-Advertiser 'through' model.""" + + ROLE_ADMIN = "Admin" + ROLE_MANAGER = "Manager" + ROLE_REPORTER = "Reporter" + ROLES = (ROLE_ADMIN, ROLE_MANAGER, ROLE_REPORTER) + + user = models.ForeignKey(User, on_delete=models.CASCADE) + advertiser = models.ForeignKey(Advertiser, on_delete=models.CASCADE) + role = models.CharField( + max_length=100, + choices=( + (ROLE_ADMIN, _(ROLE_ADMIN)), + (ROLE_MANAGER, _(ROLE_MANAGER)), + (ROLE_REPORTER, _(ROLE_REPORTER)), + ), + default=ROLE_ADMIN, + ) + + class Meta: + # This was migrated from a regular many-to-many + # To do that, we needed to start with the same table + db_table = "adserver_auth_user_advertisers" + + +class UserPublisherMember(models.Model): + """User-Publisher 'through' model.""" + + ROLE_ADMIN = "Admin" + ROLE_MANAGER = "Manager" + ROLE_REPORTER = "Reporter" + ROLES = (ROLE_ADMIN, ROLE_MANAGER, ROLE_REPORTER) + + user = models.ForeignKey(User, on_delete=models.CASCADE) + publisher = models.ForeignKey(Publisher, on_delete=models.CASCADE) + role = models.CharField( + max_length=100, + choices=( + (ROLE_ADMIN, _(ROLE_ADMIN)), + (ROLE_MANAGER, _(ROLE_MANAGER)), + (ROLE_REPORTER, _(ROLE_REPORTER)), + ), + default=ROLE_ADMIN, + ) + + class Meta: + # This was migrated from a regular many-to-many + # To do that, we needed to start with the same table + db_table = "adserver_auth_user_publishers" diff --git a/adserver/forms.py b/adserver/forms.py index 4f1b6a8a..834023ed 100644 --- a/adserver/forms.py +++ b/adserver/forms.py @@ -36,6 +36,7 @@ from django.utils.translation import gettext from django.utils.translation import gettext_lazy as _ +from .auth.models import UserAdvertiserMember from .models import Advertisement from .models import Campaign from .models import Flight @@ -1467,6 +1468,14 @@ class InviteUserForm(forms.ModelForm): without a duplicate being created. """ + role = forms.ChoiceField( + required=True, + # This form works for both publishers and advertisers + # Currently, the roles are the same for both + # If that ever changes, this will need an update + choices=((r, r) for r in UserAdvertiserMember.ROLES), + ) + def __init__(self, *args, **kwargs): """Add the form helper and customize the look of the form.""" super().__init__(*args, **kwargs) @@ -1476,6 +1485,7 @@ def __init__(self, *args, **kwargs): "", Field("name"), Field("email", placeholder="user@yourdomain.com"), + Field("role"), css_class="my-3", ), Submit("submit", "Send invite"), diff --git a/adserver/mixins.py b/adserver/mixins.py index 9a38baf0..202352a9 100644 --- a/adserver/mixins.py +++ b/adserver/mixins.py @@ -9,10 +9,11 @@ from django.core.paginator import Paginator from django.db import connection from django.db import models -from django.shortcuts import get_object_or_404 from django.utils.functional import cached_property from django.utils.translation import gettext_lazy as _ +from .auth.models import UserAdvertiserMember +from .auth.models import UserPublisherMember from .constants import ALL_CAMPAIGN_TYPES from .constants import CAMPAIGN_TYPES from .models import Advertiser @@ -34,38 +35,84 @@ class AdvertiserAccessMixin: """Mixin for checking advertiser access that works with the ``UserPassesTestMixin``.""" advertiser_slug_parameter = "advertiser_slug" + allowed_roles = ( + UserAdvertiserMember.ROLE_ADMIN, + UserAdvertiserMember.ROLE_MANAGER, + UserAdvertiserMember.ROLE_REPORTER, + ) + + def check_advertiser_role(self, advertiser): + return self.request.user.get_advertiser_role(advertiser) in self.allowed_roles def test_func(self): """The user must have access on the advertiser or be staff.""" if self.request.user.is_anonymous: return False - advertiser = get_object_or_404( - Advertiser, slug=self.kwargs[self.advertiser_slug_parameter] - ) - return ( - self.request.user.is_staff - or advertiser in self.request.user.advertisers.all() - ) + advertiser = Advertiser.objects.filter( + slug=self.kwargs[self.advertiser_slug_parameter] + ).first() + if not advertiser: + return False + + return self.request.user.is_staff or self.check_advertiser_role(advertiser) + + +class AdvertiserAdminAccessMixin(AdvertiserAccessMixin): + """Mixin for checking advertiser ADMIN role that works with the ``UserPassesTestMixin``.""" + + allowed_roles = (UserAdvertiserMember.ROLE_ADMIN,) + + +class AdvertiserManagerAccessMixin(AdvertiserAccessMixin): + """Mixin for checking advertiser Manager or Admin role that works with the ``UserPassesTestMixin``.""" + + allowed_roles = ( + UserAdvertiserMember.ROLE_ADMIN, + UserAdvertiserMember.ROLE_MANAGER, + ) class PublisherAccessMixin: """Mixin for checking publisher access that works with the ``UserPassesTestMixin``.""" publisher_slug_parameter = "publisher_slug" + allowed_roles = ( + UserPublisherMember.ROLE_ADMIN, + UserPublisherMember.ROLE_MANAGER, + UserPublisherMember.ROLE_REPORTER, + ) + + def check_publisher_role(self, publisher): + return self.request.user.get_publisher_role(publisher) in self.allowed_roles def test_func(self): """The user must have access on the publisher or be staff.""" if self.request.user.is_anonymous: return False - publisher = get_object_or_404( - Publisher, slug=self.kwargs[self.publisher_slug_parameter] - ) - return ( - self.request.user.is_staff - or publisher in self.request.user.publishers.all() - ) + publisher = Publisher.objects.filter( + slug=self.kwargs[self.publisher_slug_parameter] + ).first() + if not publisher: + return False + + return self.request.user.is_staff or self.check_publisher_role(publisher) + + +class PublisherAdminAccessMixin(PublisherAccessMixin): + """Mixin for checking publisher ADMIN role that works with the ``UserPassesTestMixin``.""" + + allowed_roles = (UserPublisherMember.ROLE_ADMIN,) + + +class PublisherManagerAccessMixin(PublisherAccessMixin): + """Mixin for checking publisher Manager or Admin role that works with the ``UserPassesTestMixin``.""" + + allowed_roles = ( + UserPublisherMember.ROLE_ADMIN, + UserPublisherMember.ROLE_MANAGER, + ) class AdvertisementValidateLinkMixin: diff --git a/adserver/templates/adserver/advertiser/advertisement-detail.html b/adserver/templates/adserver/advertiser/advertisement-detail.html index ba2a2c7f..6685be0d 100644 --- a/adserver/templates/adserver/advertiser/advertisement-detail.html +++ b/adserver/templates/adserver/advertiser/advertisement-detail.html @@ -2,6 +2,7 @@ {% load i18n %} {% load static %} {% load humanize %} +{% load ad_extras %} {% block title %}{% trans 'Advertisement: ' %}{{ advertisement.name }}{% endblock %} @@ -17,6 +18,8 @@ {% block content_container %} +{% advertiser_manager_role request.user advertiser as has_advertiser_edit_permission %} +

{% block heading %}{% trans 'Advertisement: ' %}{{ advertisement.name }}{% endblock heading %}

{% if not advertisement.live or not advertisement.flight.live %} @@ -61,9 +64,11 @@

{% block heading %}{% trans 'Advertisement: ' %}{{ advertisement.name }}{% e
+ {% if has_advertiser_edit_permission %}
{% trans 'Edit advertisement' %}
+ {% endif %}
{% endblock content_container %} diff --git a/adserver/templates/adserver/advertiser/flight-detail.html b/adserver/templates/adserver/advertiser/flight-detail.html index f62d2ba2..40f49af7 100644 --- a/adserver/templates/adserver/advertiser/flight-detail.html +++ b/adserver/templates/adserver/advertiser/flight-detail.html @@ -2,6 +2,7 @@ {% load i18n %} {% load static %} {% load humanize %} +{% load ad_extras %} {% block title %}{{ flight.name }}{% endblock %} @@ -16,6 +17,8 @@ {% block content_container %} +{% advertiser_manager_role request.user advertiser as has_advertiser_edit_permission %} +

{% block heading %}{% trans 'Flight' %}: {{ flight.name }}{% endblock heading %}

@@ -30,10 +33,12 @@

{% block heading %}{% trans 'Flight' %}: {{ flight.name }}{% endblock headin {% trans 'See full report' %} + {% if has_advertiser_edit_permission %} {% if flight.auto_renew %}{% trans 'Renewing' %}{% else %}{% trans 'Automatically renew' %}{% endif %} + {% endif %} {% if "adserver.change_flight" in perms %} @@ -53,6 +58,7 @@

{% block heading %}{% trans 'Flight' %}: {{ flight.name }}{% endblock headin
{% trans 'Live ads' %}
+ {% if has_advertiser_edit_permission %} + {% endif %}
{% if live_ads %} @@ -78,7 +85,13 @@

{% trans 'Live ads' %}
{% else %} {% url 'advertisement_create' advertiser.slug flight.slug as create_ad_url %} {% url 'advertisement_bulk_create' advertiser.slug flight.slug as bulk_create_ad_url %} -

{% blocktrans %}There are no live ads in this flight yet but you can create one or create them in bulk.{% endblocktrans %}

+

+ {% if has_advertiser_edit_permission %} + {% blocktrans %}There are no live ads in this flight yet but you can create one or create them in bulk.{% endblocktrans %} + {% else %} + {% blocktrans %}There are no live ads in this flight yet.{% endblocktrans %} + {% endif %} +

{% endif %} diff --git a/adserver/templates/adserver/advertiser/flight-list.html b/adserver/templates/adserver/advertiser/flight-list.html index c6f6e750..c53f8c98 100644 --- a/adserver/templates/adserver/advertiser/flight-list.html +++ b/adserver/templates/adserver/advertiser/flight-list.html @@ -2,6 +2,7 @@ {% load i18n %} {% load static %} {% load humanize %} +{% load ad_extras %} {% block title %}{% trans 'Manage Advertising' %}{% endblock %} @@ -15,6 +16,8 @@ {% block content_container %} +{% advertiser_manager_role request.user advertiser as has_advertiser_edit_permission %} +

{% block heading %}{% trans "Manage advertising flights" %}{% endblock heading %}

@@ -25,10 +28,12 @@

{% block heading %}{% trans "Manage advertising flights" %}{% endblock headi {% trans 'Create flight' %} {% endif %} + {% if has_advertiser_edit_permission %} {% trans 'Request a new flight' %} + {% endif %} diff --git a/adserver/templates/adserver/advertiser/users.html b/adserver/templates/adserver/advertiser/users.html index 40c236a1..a41db4bf 100644 --- a/adserver/templates/adserver/advertiser/users.html +++ b/adserver/templates/adserver/advertiser/users.html @@ -1,6 +1,7 @@ {% extends "adserver/advertiser/overview.html" %} {% load crispy_forms_tags %} {% load i18n %} +{% load ad_extras %} {% block title %}{% trans 'Authorized Users' %} - {{ advertiser }}{% endblock %} @@ -13,35 +14,49 @@ {% block content_container %} + {% advertiser_admin_role request.user advertiser as has_advertiser_admin_permission %} +

{% blocktrans %}Authorized users for {{ advertiser }}{% endblocktrans %}

-

{% trans 'These are users who have access to manage ads and view reports.' %}

+

{% trans 'These are users who have access to manage ads and view reports. The role levels are:' %}

+
+
{% trans 'Admin' %}
+
{% trans 'Can invite new users to collaborate as well as all permissions below.' %}
+
{% trans 'Manager' %}
+
{% trans 'Can manage advertisements and request new flights as well as all permissions below.' %}
+
{% trans 'Reporter' %}
+
{% trans 'Can only view reports but not change any ads or flights.' %}
+
- {% if users %} + {% if members %}
+ - {% for user in users %} + {% for member in members %} - - + + + diff --git a/adserver/templates/adserver/base.html b/adserver/templates/adserver/base.html index e7991522..f303b8f4 100644 --- a/adserver/templates/adserver/base.html +++ b/adserver/templates/adserver/base.html @@ -1,5 +1,6 @@ {% extends 'base.html' %} {% load i18n %} +{% load ad_extras %} {% block body_classes %}adserver-dashboard{% endblock body_classes %} @@ -143,6 +144,7 @@
{{ advertiser }}
{% elif publisher %} + {% publisher_admin_role request.user publisher as has_publisher_admin_permission %}
{{ publisher }}
{% trans 'Name' %} {% trans 'Email' %}{% trans 'Role' %} {% trans 'Options' %}
{{ user.name }}{{ user.email }}{{ member.user.name }}{{ member.user.email }}{{ member.role }} - {% if request.user.id != user.id %} - {% trans 'remove' %} + {% if has_advertiser_admin_permission and request.user.id != member.user.id %} + {% trans 'remove' %} {% endif %}
{% else %} -

{% blocktrans %}There are no fallback ads yet but you can create one.{% endblocktrans %}

+

+ {% if has_publisher_edit_permission %} + {% blocktrans %}There are no fallback ads yet but you can create one.{% endblocktrans %} + {% else %} + {% blocktrans %}There are no fallback ads yet.{% endblocktrans %} + {% endif %} +

{% endif %}
diff --git a/adserver/templates/adserver/publisher/users.html b/adserver/templates/adserver/publisher/users.html index 289dca04..9834b32d 100644 --- a/adserver/templates/adserver/publisher/users.html +++ b/adserver/templates/adserver/publisher/users.html @@ -1,6 +1,7 @@ {% extends "adserver/publisher/overview.html" %} {% load crispy_forms_tags %} {% load i18n %} +{% load ad_extras %} {% block title %}{% trans 'Authorized Users' %} - {{ publisher }}{% endblock %} @@ -13,35 +14,51 @@ {% block content_container %} + {% publisher_admin_role request.user publisher as has_publisher_admin_permission %} +

{% blocktrans %}Authorized users for {{ publisher }}{% endblocktrans %}

-

{% trans 'These are users who have access to manage ads and view reports.' %}

+

{% trans 'These are users who have access to manage ads and view reports. The role levels are:' %}

+
+
{% trans 'Admin' %}
+
{% trans 'Can invite new users to collaborate, update settings, as well as all permissions below.' %}
+
{% trans 'Manager' %}
+
{% trans 'Can manage fallback ads as well as all permissions below.' %}
+
{% trans 'Reporter' %}
+
{% trans 'Can only view reports but not change anything.' %}
+
- {% if users %} + {% if members %}
+ - {% for user in users %} + {% for member in members %} - - + + + diff --git a/adserver/templatetags/ad_extras.py b/adserver/templatetags/ad_extras.py index 3e48edff..b75055ae 100644 --- a/adserver/templatetags/ad_extras.py +++ b/adserver/templatetags/ad_extras.py @@ -1,9 +1,12 @@ """Custom template tags for advertisements.""" +import logging + from django import template from django.utils.safestring import mark_safe +log = logging.getLogger(__name__) # noqa register = template.Library() @@ -14,3 +17,43 @@ def advertisement_preview(ad, ad_type=None): ad_type = ad.ad_types.first() return mark_safe(ad.render_ad(ad_type, preview=True)) + + +@register.simple_tag +def advertiser_manager_role(user, advertiser): + """ + Returns True if the user has manager or higher role on this advertiser and False otherwise. + + Return True for staff. + """ + return user.is_staff or user.has_advertiser_manager_permission(advertiser) + + +@register.simple_tag +def advertiser_admin_role(user, advertiser): + """ + Returns True if the user has admin role on this advertiser and False otherwise. + + Return True for staff. + """ + return user.is_staff or user.has_advertiser_admin_permission(advertiser) + + +@register.simple_tag +def publisher_manager_role(user, publisher): + """ + Returns True if the user has manager or higher role on this publisher and False otherwise. + + Return True for staff. + """ + return user.is_staff or user.has_publisher_manager_permission(publisher) + + +@register.simple_tag +def publisher_admin_role(user, publisher): + """ + Returns True if the user has admin role on this publisher and False otherwise. + + Return True for staff. + """ + return user.is_staff or user.has_publisher_admin_permission(publisher) diff --git a/adserver/tests/test_advertiser_dashboard.py b/adserver/tests/test_advertiser_dashboard.py index ef23b6dc..ac459b87 100644 --- a/adserver/tests/test_advertiser_dashboard.py +++ b/adserver/tests/test_advertiser_dashboard.py @@ -27,6 +27,7 @@ from ..tasks import daily_update_advertisers from ..utils import get_ad_day from .common import ONE_PIXEL_PNG_BYTES +from ..auth.models import UserAdvertiserMember User = get_user_model() @@ -205,6 +206,15 @@ def test_flight_list_view(self): self.assertEqual(response.status_code, 200) self.assertContains(response, self.flight.name) + # Make it a reporter who can't request a new flight + member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) + member.role = UserAdvertiserMember.ROLE_REPORTER + member.save() + + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertNotContains(response, "Request a new flight") + def test_flight_detail_view(self): url = reverse( "flight_detail", @@ -235,6 +245,15 @@ def test_flight_detail_view(self): self.assertEqual(response.status_code, 200) self.assertContains(response, self.ad1.name) + # Make it a reporter who can't edit + member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) + member.role = UserAdvertiserMember.ROLE_REPORTER + member.save() + + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertNotContains(response, "Create advertisement") + def test_flight_detail_metadata(self): url = reverse( "flight_detail", @@ -444,6 +463,18 @@ def test_flight_autorenew_view(self): # Regular user - access to this advertiser self.client.force_login(self.user) + + # Make it a reporter who can't access + member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) + member.role = UserAdvertiserMember.ROLE_REPORTER + member.save() + + resp = self.client.get(url) + self.assertEqual(resp.status_code, 403) + + member.role = UserAdvertiserMember.ROLE_MANAGER + member.save() + resp = self.client.get(url) self.assertEqual(resp.status_code, 200) self.assertContains(resp, "Flight auto-renewal") @@ -577,6 +608,19 @@ def test_flight_request_view(self): self.assertTrue(resp["location"].startswith("/accounts/login/")) # Regular user - access to this advertiser + self.client.force_login(self.user) + + # Make it a reporter who can't access + member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) + member.role = UserAdvertiserMember.ROLE_REPORTER + member.save() + + resp = self.client.get(url) + self.assertEqual(resp.status_code, 403) + + member.role = UserAdvertiserMember.ROLE_MANAGER + member.save() + self.client.force_login(self.user) resp = self.client.get(url) self.assertEqual(resp.status_code, 200) @@ -683,6 +727,15 @@ def test_ad_detail_view(self): self.assertEqual(response.status_code, 200) self.assertContains(response, self.ad1.name) + # Make it a reporter who can't access + member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) + member.role = UserAdvertiserMember.ROLE_REPORTER + member.save() + + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertNotContains(response, "Edit advertisement") + def test_ad_update_view(self): url = reverse( "advertisement_update", @@ -699,6 +752,18 @@ def test_ad_update_view(self): self.assertTrue(response["location"].startswith("/accounts/login/")) self.client.force_login(self.user) + + # Make it a reporter who can't access + member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) + member.role = UserAdvertiserMember.ROLE_REPORTER + member.save() + + response = self.client.get(url) + self.assertEqual(response.status_code, 403) + + member.role = UserAdvertiserMember.ROLE_MANAGER + member.save() + response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertContains(response, self.ad1.name) @@ -734,6 +799,18 @@ def test_ad_create_view(self): self.assertTrue(response["location"].startswith("/accounts/login/")) self.client.force_login(self.user) + + # Make it a reporter who can't access + member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) + member.role = UserAdvertiserMember.ROLE_REPORTER + member.save() + + response = self.client.get(url) + self.assertEqual(response.status_code, 403) + + member.role = UserAdvertiserMember.ROLE_MANAGER + member.save() + response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertContains(response, "Create advertisement") @@ -825,6 +902,18 @@ def test_ad_copy_view(self): self.assertTrue(response["location"].startswith("/accounts/login/")) self.client.force_login(self.user) + + # Make it a reporter who can't access + member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) + member.role = UserAdvertiserMember.ROLE_REPORTER + member.save() + + response = self.client.get(url) + self.assertEqual(response.status_code, 403) + + member.role = UserAdvertiserMember.ROLE_MANAGER + member.save() + response = self.client.get(url) self.assertContains(response, "Re-use your previous ads") self.assertContains(response, self.ad1.name) @@ -886,11 +975,17 @@ def test_authorized_users(self): self.assertEqual(response.status_code, 302) self.assertTrue(response["location"].startswith("/accounts/login/")) + # Make it a manager who can't invite users + member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) + member.role = UserAdvertiserMember.ROLE_MANAGER + member.save() + self.client.force_login(self.user) response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertContains(response, self.user.name) self.assertContains(response, self.user.email) + self.assertNotContains(response, "Invite user") self.user.advertisers.remove(self.advertiser) @@ -943,7 +1038,17 @@ def test_authorized_users_invite(self): self.assertEqual(response.status_code, 302) self.assertTrue(response["location"].startswith("/accounts/login/")) + # Make it a manager who can't invite users + member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) + member.role = UserAdvertiserMember.ROLE_MANAGER + member.save() + self.client.force_login(self.user) + response = self.client.get(url) + self.assertEqual(response.status_code, 403) + + member.role = UserAdvertiserMember.ROLE_ADMIN + member.save() response = self.client.get(url) self.assertEqual(response.status_code, 200) @@ -951,7 +1056,7 @@ def test_authorized_users_invite(self): response = self.client.post( url, - data={"name": "Another User", "email": "another@example.com"}, + data={"name": "Another User", "email": "another@example.com", "role": UserAdvertiserMember.ROLE_MANAGER}, follow=True, ) self.assertEqual(response.status_code, 200) @@ -972,7 +1077,7 @@ def test_authorized_users_invite_existing(self): response = self.client.post( url, - data={"name": name, "email": email}, + data={"name": name, "email": email, "role": UserAdvertiserMember.ROLE_MANAGER}, follow=True, ) self.assertEqual(response.status_code, 200) @@ -982,7 +1087,7 @@ def test_authorized_users_invite_existing(self): # Invite the same user again to check that the user isn't created again response = self.client.post( url, - data={"name": "Yet Another User", "email": email}, + data={"name": "Yet Another User", "email": email, "role": UserAdvertiserMember.ROLE_MANAGER}, follow=True, ) self.assertEqual(response.status_code, 200) diff --git a/adserver/tests/test_publisher_dashboard.py b/adserver/tests/test_publisher_dashboard.py index 741bd4da..a9cad891 100644 --- a/adserver/tests/test_publisher_dashboard.py +++ b/adserver/tests/test_publisher_dashboard.py @@ -19,6 +19,7 @@ from ..models import Publisher from ..models import PublisherPayout from ..tasks import daily_update_publishers +from ..auth.models import UserPublisherMember class TestPublisherDashboardViews(TestCase): @@ -152,7 +153,18 @@ def test_publisher_settings(self): self.assertEqual(resp.status_code, 302) self.assertTrue(resp["location"].startswith("/accounts/login/")) - self.client.force_login(self.staff_user) + member = UserPublisherMember.objects.create( + user=self.user, + publisher=self.publisher1, + role=UserPublisherMember.ROLE_MANAGER, + ) + self.client.force_login(self.user) + + resp = self.client.get(url) + self.assertEqual(resp.status_code, 403) + + member.role = UserPublisherMember.ROLE_ADMIN + member.save() resp = self.client.get(url) self.assertEqual(resp.status_code, 200) @@ -391,15 +403,20 @@ def test_authorized_users(self): self.assertEqual(response.status_code, 302) self.assertTrue(response["location"].startswith("/accounts/login/")) - self.user.publishers.add(self.publisher1) + member = UserPublisherMember.objects.create( + user=self.user, + publisher=self.publisher1, + role=UserPublisherMember.ROLE_MANAGER, + ) self.client.force_login(self.user) response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertContains(response, self.user.name) self.assertContains(response, self.user.email) + self.assertNotContains(response, "Invite user") - self.user.publishers.remove(self.publisher1) + member.delete() self.client.force_login(self.staff_user) response = self.client.get(url) @@ -453,16 +470,27 @@ def test_authorized_users_invite(self): self.assertEqual(response.status_code, 302) self.assertTrue(response["location"].startswith("/accounts/login/")) - self.user.publishers.add(self.publisher1) + member = UserPublisherMember.objects.create( + user=self.user, + publisher=self.publisher1, + role=UserPublisherMember.ROLE_MANAGER, + ) + self.client.force_login(self.user) + response = self.client.get(url) + self.assertEqual(response.status_code, 403) + + member.role = UserPublisherMember.ROLE_ADMIN + member.save() + response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertContains(response, "Invite user to") response = self.client.post( url, - data={"name": "Another User", "email": "another@example.com"}, + data={"name": "Another User", "email": "another@example.com", "role": UserPublisherMember.ROLE_REPORTER}, follow=True, ) self.assertEqual(response.status_code, 200) @@ -486,7 +514,7 @@ def test_authorized_users_invite_existing(self): response = self.client.post( url, - data={"name": name, "email": email}, + data={"name": name, "email": email, "role": UserPublisherMember.ROLE_REPORTER}, follow=True, ) self.assertEqual(response.status_code, 200) @@ -496,7 +524,7 @@ def test_authorized_users_invite_existing(self): # Invite the same user again to check that the user isn't created again response = self.client.post( url, - data={"name": "Yet Another User", "email": email}, + data={"name": "Yet Another User", "email": email, "role": UserPublisherMember.ROLE_REPORTER}, follow=True, ) self.assertEqual(response.status_code, 200) @@ -572,11 +600,23 @@ def test_fallback_ads_list(self): resp = self.client.get(url) self.assertEqual(resp.status_code, 403) - self.user.publishers.add(self.publisher) + member = UserPublisherMember.objects.create( + user=self.user, + publisher=self.publisher, + role=UserPublisherMember.ROLE_MANAGER, + ) resp = self.client.get(url) self.assertEqual(resp.status_code, 200) self.assertContains(resp, "Test Ad 1") + self.assertContains(resp, "Create fallback ad") + + member.role = UserPublisherMember.ROLE_REPORTER + member.save() + + resp = self.client.get(url) + self.assertEqual(resp.status_code, 200) + self.assertNotContains(resp, "Create fallback ad") def test_fallback_ads_detail(self): url = reverse( @@ -594,11 +634,23 @@ def test_fallback_ads_detail(self): resp = self.client.get(url) self.assertEqual(resp.status_code, 403) - self.user.publishers.add(self.publisher) + member = UserPublisherMember.objects.create( + user=self.user, + publisher=self.publisher, + role=UserPublisherMember.ROLE_MANAGER, + ) resp = self.client.get(url) self.assertEqual(resp.status_code, 200) self.assertContains(resp, "Test Ad 1") + self.assertContains(resp, "Edit fallback ad") + + member.role = UserPublisherMember.ROLE_REPORTER + member.save() + + resp = self.client.get(url) + self.assertEqual(resp.status_code, 200) + self.assertNotContains(resp, "Edit fallback ad") def test_fallback_ads_update(self): url = reverse( @@ -616,7 +668,18 @@ def test_fallback_ads_update(self): resp = self.client.get(url) self.assertEqual(resp.status_code, 403) - self.user.publishers.add(self.publisher) + member = UserPublisherMember.objects.create( + user=self.user, + publisher=self.publisher, + role=UserPublisherMember.ROLE_REPORTER, + ) + + # Reporters can't edit + resp = self.client.get(url) + self.assertEqual(resp.status_code, 403) + + member.role = UserPublisherMember.ROLE_MANAGER + member.save() resp = self.client.get(url) self.assertEqual(resp.status_code, 200) @@ -653,7 +716,18 @@ def test_fallback_ads_create(self): resp = self.client.get(url) self.assertEqual(resp.status_code, 403) - self.user.publishers.add(self.publisher) + member = UserPublisherMember.objects.create( + user=self.user, + publisher=self.publisher, + role=UserPublisherMember.ROLE_REPORTER, + ) + + # Reporter can't create + resp = self.client.get(url) + self.assertEqual(resp.status_code, 403) + + member.role = UserPublisherMember.ROLE_MANAGER + member.save() resp = self.client.get(url) self.assertEqual(resp.status_code, 200) diff --git a/adserver/views.py b/adserver/views.py index 36e48a21..3d849eaa 100644 --- a/adserver/views.py +++ b/adserver/views.py @@ -53,6 +53,8 @@ from rest_framework.authtoken.models import Token from user_agents import parse as parse_user_agent +from .auth.models import UserAdvertiserMember +from .auth.models import UserPublisherMember from .constants import ALL_CAMPAIGN_TYPES from .constants import CAMPAIGN_TYPES from .constants import CLICKS @@ -76,10 +78,14 @@ from .forms import SupportForm from .mixins import AdvertisementValidateLinkMixin from .mixins import AdvertiserAccessMixin +from .mixins import AdvertiserAdminAccessMixin +from .mixins import AdvertiserManagerAccessMixin from .mixins import AllReportMixin from .mixins import GeoReportMixin from .mixins import KeywordReportMixin from .mixins import PublisherAccessMixin +from .mixins import PublisherAdminAccessMixin +from .mixins import PublisherManagerAccessMixin from .mixins import ReportQuerysetMixin from .models import AdImpression from .models import AdType @@ -410,7 +416,9 @@ def get_success_url(self): ) -class FlightSetAutoRenewView(AdvertiserAccessMixin, UserPassesTestMixin, UpdateView): +class FlightSetAutoRenewView( + AdvertiserManagerAccessMixin, UserPassesTestMixin, UpdateView +): """Allow advertisers to set a flight to auto-renew or not.""" form_class = FlightAutoRenewForm @@ -520,7 +528,7 @@ def get_success_url(self): ) -class FlightRequestView(AdvertiserAccessMixin, UserPassesTestMixin, CreateView): +class FlightRequestView(AdvertiserManagerAccessMixin, UserPassesTestMixin, CreateView): """Create a new flight for an advertiser.""" form_class = FlightRequestForm @@ -658,7 +666,7 @@ def get_object(self, queryset=None): class AdvertisementUpdateView( - AdvertiserAccessMixin, + AdvertiserManagerAccessMixin, UserPassesTestMixin, AdvertisementValidateLinkMixin, UpdateView, @@ -712,7 +720,7 @@ def get_success_url(self): class AdvertisementCreateView( - AdvertiserAccessMixin, + AdvertiserManagerAccessMixin, UserPassesTestMixin, AdvertisementValidateLinkMixin, CreateView, @@ -772,7 +780,7 @@ def get_success_url(self): class AdvertisementBulkCreateView( - AdvertiserAccessMixin, + AdvertiserManagerAccessMixin, UserPassesTestMixin, FormView, ): @@ -917,7 +925,9 @@ def get_error_url(self): ) -class AdvertisementCopyView(AdvertiserAccessMixin, UserPassesTestMixin, FormView): +class AdvertisementCopyView( + AdvertiserManagerAccessMixin, UserPassesTestMixin, FormView +): """Create a copy of an existing ad.""" form_class = AdvertisementCopyForm @@ -1695,8 +1705,8 @@ class AdvertiserAuthorizedUsersView( ): """Authorized users for an advertiser.""" - context_object_name = "users" - model = get_user_model() + context_object_name = "members" + model = UserAdvertiserMember template_name = "adserver/advertiser/users.html" def get_context_data(self, **kwargs): @@ -1708,11 +1718,15 @@ def get_queryset(self): self.advertiser = get_object_or_404( Advertiser, slug=self.kwargs.get("advertiser_slug", "") ) - return self.advertiser.user_set.all() + return ( + UserAdvertiserMember.objects.filter(advertiser=self.advertiser) + .select_related() + .all() + ) class AdvertiserAuthorizedUsersInviteView( - AdvertiserAccessMixin, UserPassesTestMixin, CreateView + AdvertiserAdminAccessMixin, UserPassesTestMixin, CreateView ): """Invite additional authorized users for an advertiser.""" @@ -1728,7 +1742,15 @@ def dispatch(self, request, *args, **kwargs): def form_valid(self, form): result = super().form_valid(form) - self.object.advertisers.add(self.advertiser) + + # Add m2m and role + role = form.cleaned_data["role"] + UserAdvertiserMember.objects.create( + advertiser=self.advertiser, + user=self.object, + role=role, + ) + messages.success( self.request, _("Successfully invited %(user)s to %(advertiser)s") @@ -1748,7 +1770,7 @@ def get_success_url(self): class AdvertiserAuthorizedUsersRemoveView( - AdvertiserAccessMixin, UserPassesTestMixin, TemplateView + AdvertiserAdminAccessMixin, UserPassesTestMixin, TemplateView ): """ Remove authorized users for an advertiser. @@ -2150,7 +2172,7 @@ def get_object(self, queryset=None): class PublisherFallbackAdsUpdateView( - FallbackAdsMixin, PublisherAccessMixin, UserPassesTestMixin, UpdateView + FallbackAdsMixin, PublisherManagerAccessMixin, UserPassesTestMixin, UpdateView ): """Update a fallback ad.""" @@ -2193,7 +2215,7 @@ def get_success_url(self): class PublisherFallbackAdsCreateView( - FallbackAdsMixin, PublisherAccessMixin, UserPassesTestMixin, CreateView + FallbackAdsMixin, PublisherManagerAccessMixin, UserPassesTestMixin, CreateView ): """Create a fallback ad.""" @@ -2239,7 +2261,7 @@ def get_success_url(self): ) -class PublisherSettingsView(PublisherAccessMixin, UserPassesTestMixin, UpdateView): +class PublisherSettingsView(PublisherAdminAccessMixin, UserPassesTestMixin, UpdateView): """Settings configuration for a publisher.""" form_class = PublisherSettingsForm @@ -2270,7 +2292,7 @@ def get_success_url(self): class PublisherStripeOauthConnectView( - PublisherAccessMixin, UserPassesTestMixin, RedirectView + PublisherAdminAccessMixin, UserPassesTestMixin, RedirectView ): """Redirect the user to the correct Stripe connect URL for the publisher.""" @@ -2559,7 +2581,7 @@ def get_context_data(self, **kwargs): # pylint: disable=too-many-locals class PublisherAuthorizedUsersView(PublisherAccessMixin, UserPassesTestMixin, ListView): """Authorized users for a publisher.""" - context_object_name = "users" + context_object_name = "members" model = get_user_model() template_name = "adserver/publisher/users.html" @@ -2572,11 +2594,15 @@ def get_queryset(self): self.publisher = get_object_or_404( Publisher, slug=self.kwargs.get("publisher_slug", "") ) - return self.publisher.user_set.all() + return ( + UserPublisherMember.objects.filter(publisher=self.publisher) + .select_related() + .all() + ) class PublisherAuthorizedUsersInviteView( - PublisherAccessMixin, UserPassesTestMixin, CreateView + PublisherAdminAccessMixin, UserPassesTestMixin, CreateView ): """Invite additional authorized users for a publisher.""" @@ -2592,7 +2618,15 @@ def dispatch(self, request, *args, **kwargs): def form_valid(self, form): result = super().form_valid(form) - self.object.publishers.add(self.publisher) + + # Add m2m and role + role = form.cleaned_data["role"] + UserPublisherMember.objects.create( + publisher=self.publisher, + user=self.object, + role=role, + ) + messages.success( self.request, _("Successfully invited %(user)s to %(publisher)s") @@ -2612,7 +2646,7 @@ def get_success_url(self): class PublisherAuthorizedUsersRemoveView( - PublisherAccessMixin, UserPassesTestMixin, TemplateView + PublisherAdminAccessMixin, UserPassesTestMixin, TemplateView ): """ Remove authorized users for a publisher.
{% trans 'Name' %} {% trans 'Email' %}{% trans 'Role' %} {% trans 'Options' %}
{{ user.name }}{{ user.email }}{{ member.user.name }}{{ member.user.email }}{{ member.role }} - {% if request.user.id != user.id %} - {% trans 'remove' %} + {% if has_publisher_admin_permission %} + {% if request.user.id != member.user.id %} + {% trans 'remove' %} + {% endif %} {% endif %}