From 90a4a762e230db6f87b13add65ae11995e8ce5c7 Mon Sep 17 00:00:00 2001 From: David Fischer Date: Thu, 14 Nov 2024 16:15:29 -0800 Subject: [PATCH 1/3] Add roles for advertisers and publishers - Add 3 permission levels for advertisers and publishers - The migration will default everybody to the highest permission - Role permissions are checked in the UI and in the backend - Admin users can change publisher settings, and invite users. - Managers can edit things - Reporters can't change anything. --- adserver/auth/admin.py | 19 +++- .../0009_user_advertiser_publisher_roles.py | 70 ++++++++++++++ adserver/auth/models.py | 56 ++++++++++- adserver/forms.py | 10 ++ adserver/mixins.py | 59 ++++++++++-- .../advertiser/advertisement-detail.html | 6 ++ .../adserver/advertiser/flight-detail.html | 16 +++- .../adserver/advertiser/flight-list.html | 6 ++ .../templates/adserver/advertiser/users.html | 32 +++++-- adserver/templates/adserver/base.html | 4 + adserver/templates/adserver/dashboard.html | 1 + .../publisher/fallback-ads-detail.html | 6 ++ .../adserver/publisher/fallback-ads-list.html | 14 ++- .../templates/adserver/publisher/users.html | 32 +++++-- adserver/templatetags/ad_extras.py | 52 ++++++++++ adserver/tests/test_advertiser_dashboard.py | 84 +++++++++++++++- adserver/tests/test_publisher_dashboard.py | 96 ++++++++++++++++--- adserver/views.py | 74 ++++++++++---- 18 files changed, 574 insertions(+), 63 deletions(-) create mode 100644 adserver/auth/migrations/0009_user_advertiser_publisher_roles.py 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..cea8c166 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( @@ -146,3 +150,51 @@ 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: + 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: + db_table = "adserver_auth_user_publishers" diff --git a/adserver/forms.py b/adserver/forms.py index 829cc3c5..6e3ef925 100644 --- a/adserver/forms.py +++ b/adserver/forms.py @@ -29,6 +29,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 @@ -1322,6 +1323,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) @@ -1331,6 +1340,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..87116da5 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,40 +35,80 @@ 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 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() + or self.request.user.useradvertisermember_set.filter( + advertiser__slug=self.kwargs[self.advertiser_slug_parameter], + role__in=self.allowed_roles, + ).exists() ) +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 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() + or self.request.user.userpublishermember_set.filter( + publisher__slug=self.kwargs[self.publisher_slug_parameter], + role__in=self.allowed_roles, + ).exists() ) +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: """ Mixin for validating the landing page returns a 200. diff --git a/adserver/templates/adserver/advertiser/advertisement-detail.html b/adserver/templates/adserver/advertiser/advertisement-detail.html index ba2a2c7f..8cb50ab0 100644 --- a/adserver/templates/adserver/advertiser/advertisement-detail.html +++ b/adserver/templates/adserver/advertiser/advertisement-detail.html @@ -2,6 +2,10 @@ {% load i18n %} {% load static %} {% load humanize %} +{% load ad_extras %} + + +{% advertiser_role request.user advertiser as user_advertiser_role %} {% block title %}{% trans 'Advertisement: ' %}{{ advertisement.name }}{% endblock %} @@ -61,9 +65,11 @@

{% block heading %}{% trans 'Advertisement: ' %}{{ advertisement.name }}{% e
+ {% if request.user.is_staff or user_advertiser_role == "Admin" or user_advertiser_role == "Manager" %}
{% 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 2cfdab0d..7364cb8d 100644 --- a/adserver/templates/adserver/advertiser/flight-detail.html +++ b/adserver/templates/adserver/advertiser/flight-detail.html @@ -2,6 +2,10 @@ {% load i18n %} {% load static %} {% load humanize %} +{% load ad_extras %} + + +{% advertiser_role request.user advertiser as user_advertiser_role %} {% block title %}{{ flight.name }}{% endblock %} @@ -30,10 +34,12 @@

{% block heading %}{% trans 'Flight' %}: {{ flight.name }}{% endblock headin {% trans 'See full report' %} + {% if request.user.is_staff or user_advertiser_role == "Admin" or user_advertiser_role == "Manager" %} {% if flight.auto_renew %}{% trans 'Renewing' %}{% else %}{% trans 'Automatically renew' %}{% endif %} + {% endif %} {% if "adserver.change_flight" in perms %} @@ -54,6 +60,7 @@

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

{% trans 'Live ads' %}
 + {% endif %} {% if live_ads %} @@ -73,7 +81,13 @@

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

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

+

+ {% if user_advertiser_role == "Reporter" %} + {% blocktrans %}There are no live ads in this flight yet.{% endblocktrans %} + {% else %} + {% blocktrans %}There are no live ads in this flight yet but you can create one.{% endblocktrans %} + {% endif %} +

{% endif %} diff --git a/adserver/templates/adserver/advertiser/flight-list.html b/adserver/templates/adserver/advertiser/flight-list.html index acb441eb..939ce156 100644 --- a/adserver/templates/adserver/advertiser/flight-list.html +++ b/adserver/templates/adserver/advertiser/flight-list.html @@ -2,6 +2,10 @@ {% load i18n %} {% load static %} {% load humanize %} +{% load ad_extras %} + + +{% advertiser_role request.user advertiser as user_advertiser_role %} {% block title %}{% trans 'Manage Advertising' %}{% endblock %} @@ -25,10 +29,12 @@

{% block heading %}{% trans "Manage advertising flights" %}{% endblock headi {% trans 'Create flight' %} {% endif %} + {% if request.user.is_staff or user_advertiser_role == "Admin" or user_advertiser_role == "Manager" %} {% trans 'Request a new flight' %} + {% endif %} diff --git a/adserver/templates/adserver/advertiser/users.html b/adserver/templates/adserver/advertiser/users.html index 40c236a1..d6504f01 100644 --- a/adserver/templates/adserver/advertiser/users.html +++ b/adserver/templates/adserver/advertiser/users.html @@ -1,6 +1,10 @@ {% extends "adserver/advertiser/overview.html" %} {% load crispy_forms_tags %} {% load i18n %} +{% load ad_extras %} + + +{% advertiser_role request.user advertiser as user_advertiser_role %} {% block title %}{% trans 'Authorized Users' %} - {{ advertiser }}{% endblock %} @@ -15,33 +19,47 @@ {% block content_container %}

{% 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..68d7057e 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_role request.user publisher as user_publisher_role %}
{{ 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 request.user.is_staff or user_advertiser_role == "Admin" %} + {% if request.user.id != member.user.id %} + {% trans 'remove' %} + {% endif %} {% endif %}
{% else %} -

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

+

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

{% endif %}
diff --git a/adserver/templates/adserver/publisher/users.html b/adserver/templates/adserver/publisher/users.html index 289dca04..885dd4dd 100644 --- a/adserver/templates/adserver/publisher/users.html +++ b/adserver/templates/adserver/publisher/users.html @@ -1,6 +1,10 @@ {% extends "adserver/publisher/overview.html" %} {% load crispy_forms_tags %} {% load i18n %} +{% load ad_extras %} + + +{% publisher_role request.user publisher as user_publisher_role %} {% block title %}{% trans 'Authorized Users' %} - {{ publisher }}{% endblock %} @@ -15,33 +19,47 @@ {% block content_container %}

{% 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..f2fa7690 100644 --- a/adserver/templatetags/ad_extras.py +++ b/adserver/templatetags/ad_extras.py @@ -14,3 +14,55 @@ 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_role(user, 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(user, "_advertiser_roles"): + user._advertiser_roles = {} + + if advertiser.pk in user._advertiser_roles: + return user._advertiser_roles[advertiser.pk] + + membership = user.useradvertisermember_set.filter( + advertiser=advertiser, + ).first() + + role = None + if membership: + role = membership.role + + user._advertiser_roles[advertiser.pk] = role + return role + + +@register.simple_tag +def publisher_role(user, 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(user, "_publisher_roles"): + user._publisher_roles = {} + + if publisher.pk in user._publisher_roles: + return user._publisher_roles[publisher.pk] + + membership = user.userpublishermember_set.filter( + publisher=publisher, + ).first() + + role = None + if membership: + role = membership.role + + user._publisher_roles[publisher.pk] = role + return role diff --git a/adserver/tests/test_advertiser_dashboard.py b/adserver/tests/test_advertiser_dashboard.py index 9a273cc1..c12b9dd0 100644 --- a/adserver/tests/test_advertiser_dashboard.py +++ b/adserver/tests/test_advertiser_dashboard.py @@ -25,6 +25,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() @@ -442,6 +443,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 = "Reporter" + member.save() + + resp = self.client.get(url) + self.assertEqual(resp.status_code, 403) + + member.role = "Manager" + member.save() + resp = self.client.get(url) self.assertEqual(resp.status_code, 200) self.assertContains(resp, "Flight auto-renewal") @@ -575,6 +588,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 = "Reporter" + member.save() + + resp = self.client.get(url) + self.assertEqual(resp.status_code, 403) + + member.role = "Manager" + member.save() + self.client.force_login(self.user) resp = self.client.get(url) self.assertEqual(resp.status_code, 200) @@ -697,6 +723,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 = "Reporter" + member.save() + + response = self.client.get(url) + self.assertEqual(response.status_code, 403) + + member.role = "Manager" + member.save() + response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertContains(response, self.ad1.name) @@ -732,6 +770,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 = "Reporter" + member.save() + + response = self.client.get(url) + self.assertEqual(response.status_code, 403) + + member.role = "Manager" + member.save() + response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertContains(response, "Create advertisement") @@ -767,6 +817,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 = "Reporter" + member.save() + + response = self.client.get(url) + self.assertEqual(response.status_code, 403) + + member.role = "Manager" + member.save() + response = self.client.get(url) self.assertContains(response, "Re-use your previous ads") self.assertContains(response, self.ad1.name) @@ -828,11 +890,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 = "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) @@ -885,7 +953,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 = "Manager" + member.save() + self.client.force_login(self.user) + response = self.client.get(url) + self.assertEqual(response.status_code, 403) + + member.role = "Admin" + member.save() response = self.client.get(url) self.assertEqual(response.status_code, 200) @@ -893,7 +971,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": "Manager"}, follow=True, ) self.assertEqual(response.status_code, 200) @@ -914,7 +992,7 @@ def test_authorized_users_invite_existing(self): response = self.client.post( url, - data={"name": name, "email": email}, + data={"name": name, "email": email, "role": "Manager"}, follow=True, ) self.assertEqual(response.status_code, 200) @@ -924,7 +1002,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": "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..51dc74c3 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="Manager", + ) + self.client.force_login(self.user) + + resp = self.client.get(url) + self.assertEqual(resp.status_code, 403) + + member.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="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="Manager", + ) + self.client.force_login(self.user) + response = self.client.get(url) + self.assertEqual(response.status_code, 403) + + member.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": "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": "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": "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="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 = "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="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 = "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="Reporter", + ) + + # Reporters can't edit + resp = self.client.get(url) + self.assertEqual(resp.status_code, 403) + + member.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="Reporter", + ) + + # Reporter can't create + resp = self.client.get(url) + self.assertEqual(resp.status_code, 403) + + member.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 1de2b4e5..b415fe9a 100644 --- a/adserver/views.py +++ b/adserver/views.py @@ -49,6 +49,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 @@ -71,10 +73,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 @@ -405,7 +411,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 @@ -515,7 +523,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 @@ -653,7 +661,7 @@ def get_object(self, queryset=None): class AdvertisementUpdateView( - AdvertiserAccessMixin, + AdvertiserManagerAccessMixin, UserPassesTestMixin, AdvertisementValidateLinkMixin, UpdateView, @@ -707,7 +715,7 @@ def get_success_url(self): class AdvertisementCreateView( - AdvertiserAccessMixin, + AdvertiserManagerAccessMixin, UserPassesTestMixin, AdvertisementValidateLinkMixin, CreateView, @@ -766,7 +774,9 @@ def get_success_url(self): ) -class AdvertisementCopyView(AdvertiserAccessMixin, UserPassesTestMixin, FormView): +class AdvertisementCopyView( + AdvertiserManagerAccessMixin, UserPassesTestMixin, FormView +): """Create a copy of an existing ad.""" form_class = AdvertisementCopyForm @@ -1544,8 +1554,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): @@ -1557,11 +1567,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.""" @@ -1577,7 +1591,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") @@ -1597,7 +1619,7 @@ def get_success_url(self): class AdvertiserAuthorizedUsersRemoveView( - AdvertiserAccessMixin, UserPassesTestMixin, TemplateView + AdvertiserAdminAccessMixin, UserPassesTestMixin, TemplateView ): """ Remove authorized users for an advertiser. @@ -1999,7 +2021,7 @@ def get_object(self, queryset=None): class PublisherFallbackAdsUpdateView( - FallbackAdsMixin, PublisherAccessMixin, UserPassesTestMixin, UpdateView + FallbackAdsMixin, PublisherManagerAccessMixin, UserPassesTestMixin, UpdateView ): """Update a fallback ad.""" @@ -2042,7 +2064,7 @@ def get_success_url(self): class PublisherFallbackAdsCreateView( - FallbackAdsMixin, PublisherAccessMixin, UserPassesTestMixin, CreateView + FallbackAdsMixin, PublisherManagerAccessMixin, UserPassesTestMixin, CreateView ): """Create a fallback ad.""" @@ -2088,7 +2110,7 @@ def get_success_url(self): ) -class PublisherSettingsView(PublisherAccessMixin, UserPassesTestMixin, UpdateView): +class PublisherSettingsView(PublisherAdminAccessMixin, UserPassesTestMixin, UpdateView): """Settings configuration for a publisher.""" form_class = PublisherSettingsForm @@ -2119,7 +2141,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.""" @@ -2408,7 +2430,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" @@ -2421,11 +2443,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.""" @@ -2441,7 +2467,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") @@ -2461,7 +2495,7 @@ def get_success_url(self): class PublisherAuthorizedUsersRemoveView( - PublisherAccessMixin, UserPassesTestMixin, TemplateView + PublisherAdminAccessMixin, UserPassesTestMixin, TemplateView ): """ Remove authorized users for a publisher. From d15884ea6bbfb506da26f2fa14bf17696bfa92cc Mon Sep 17 00:00:00 2001 From: David Fischer Date: Mon, 18 Nov 2024 15:43:13 -0800 Subject: [PATCH 2/3] Use role constants in tests --- adserver/tests/test_advertiser_dashboard.py | 32 ++++++++++----------- adserver/tests/test_publisher_dashboard.py | 32 ++++++++++----------- 2 files changed, 32 insertions(+), 32 deletions(-) diff --git a/adserver/tests/test_advertiser_dashboard.py b/adserver/tests/test_advertiser_dashboard.py index c12b9dd0..55de382e 100644 --- a/adserver/tests/test_advertiser_dashboard.py +++ b/adserver/tests/test_advertiser_dashboard.py @@ -446,13 +446,13 @@ def test_flight_autorenew_view(self): # Make it a reporter who can't access member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) - member.role = "Reporter" + member.role = UserAdvertiserMember.ROLE_REPORTER member.save() resp = self.client.get(url) self.assertEqual(resp.status_code, 403) - member.role = "Manager" + member.role = UserAdvertiserMember.ROLE_MANAGER member.save() resp = self.client.get(url) @@ -592,13 +592,13 @@ def test_flight_request_view(self): # Make it a reporter who can't access member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) - member.role = "Reporter" + member.role = UserAdvertiserMember.ROLE_REPORTER member.save() resp = self.client.get(url) self.assertEqual(resp.status_code, 403) - member.role = "Manager" + member.role = UserAdvertiserMember.ROLE_MANAGER member.save() self.client.force_login(self.user) @@ -726,13 +726,13 @@ def test_ad_update_view(self): # Make it a reporter who can't access member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) - member.role = "Reporter" + member.role = UserAdvertiserMember.ROLE_REPORTER member.save() response = self.client.get(url) self.assertEqual(response.status_code, 403) - member.role = "Manager" + member.role = UserAdvertiserMember.ROLE_MANAGER member.save() response = self.client.get(url) @@ -773,13 +773,13 @@ def test_ad_create_view(self): # Make it a reporter who can't access member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) - member.role = "Reporter" + member.role = UserAdvertiserMember.ROLE_REPORTER member.save() response = self.client.get(url) self.assertEqual(response.status_code, 403) - member.role = "Manager" + member.role = UserAdvertiserMember.ROLE_MANAGER member.save() response = self.client.get(url) @@ -820,13 +820,13 @@ def test_ad_copy_view(self): # Make it a reporter who can't access member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) - member.role = "Reporter" + member.role = UserAdvertiserMember.ROLE_REPORTER member.save() response = self.client.get(url) self.assertEqual(response.status_code, 403) - member.role = "Manager" + member.role = UserAdvertiserMember.ROLE_MANAGER member.save() response = self.client.get(url) @@ -892,7 +892,7 @@ def test_authorized_users(self): # Make it a manager who can't invite users member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) - member.role = "Manager" + member.role = UserAdvertiserMember.ROLE_MANAGER member.save() self.client.force_login(self.user) @@ -955,14 +955,14 @@ def test_authorized_users_invite(self): # Make it a manager who can't invite users member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser) - member.role = "Manager" + 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 = "Admin" + member.role = UserAdvertiserMember.ROLE_ADMIN member.save() response = self.client.get(url) @@ -971,7 +971,7 @@ def test_authorized_users_invite(self): response = self.client.post( url, - data={"name": "Another User", "email": "another@example.com", "role": "Manager"}, + data={"name": "Another User", "email": "another@example.com", "role": UserAdvertiserMember.ROLE_MANAGER}, follow=True, ) self.assertEqual(response.status_code, 200) @@ -992,7 +992,7 @@ def test_authorized_users_invite_existing(self): response = self.client.post( url, - data={"name": name, "email": email, "role": "Manager"}, + data={"name": name, "email": email, "role": UserAdvertiserMember.ROLE_MANAGER}, follow=True, ) self.assertEqual(response.status_code, 200) @@ -1002,7 +1002,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, "role": "Manager"}, + 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 51dc74c3..a9cad891 100644 --- a/adserver/tests/test_publisher_dashboard.py +++ b/adserver/tests/test_publisher_dashboard.py @@ -156,14 +156,14 @@ def test_publisher_settings(self): member = UserPublisherMember.objects.create( user=self.user, publisher=self.publisher1, - role="Manager", + role=UserPublisherMember.ROLE_MANAGER, ) self.client.force_login(self.user) resp = self.client.get(url) self.assertEqual(resp.status_code, 403) - member.role = "Admin" + member.role = UserPublisherMember.ROLE_ADMIN member.save() resp = self.client.get(url) @@ -406,7 +406,7 @@ def test_authorized_users(self): member = UserPublisherMember.objects.create( user=self.user, publisher=self.publisher1, - role="Manager", + role=UserPublisherMember.ROLE_MANAGER, ) self.client.force_login(self.user) @@ -473,7 +473,7 @@ def test_authorized_users_invite(self): member = UserPublisherMember.objects.create( user=self.user, publisher=self.publisher1, - role="Manager", + role=UserPublisherMember.ROLE_MANAGER, ) self.client.force_login(self.user) @@ -481,7 +481,7 @@ def test_authorized_users_invite(self): response = self.client.get(url) self.assertEqual(response.status_code, 403) - member.role = "Admin" + member.role = UserPublisherMember.ROLE_ADMIN member.save() response = self.client.get(url) @@ -490,7 +490,7 @@ def test_authorized_users_invite(self): response = self.client.post( url, - data={"name": "Another User", "email": "another@example.com", "role": "Reporter"}, + data={"name": "Another User", "email": "another@example.com", "role": UserPublisherMember.ROLE_REPORTER}, follow=True, ) self.assertEqual(response.status_code, 200) @@ -514,7 +514,7 @@ def test_authorized_users_invite_existing(self): response = self.client.post( url, - data={"name": name, "email": email, "role": "Reporter"}, + data={"name": name, "email": email, "role": UserPublisherMember.ROLE_REPORTER}, follow=True, ) self.assertEqual(response.status_code, 200) @@ -524,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, "role": "Reporter"}, + data={"name": "Yet Another User", "email": email, "role": UserPublisherMember.ROLE_REPORTER}, follow=True, ) self.assertEqual(response.status_code, 200) @@ -603,7 +603,7 @@ def test_fallback_ads_list(self): member = UserPublisherMember.objects.create( user=self.user, publisher=self.publisher, - role="Manager", + role=UserPublisherMember.ROLE_MANAGER, ) resp = self.client.get(url) @@ -611,7 +611,7 @@ def test_fallback_ads_list(self): self.assertContains(resp, "Test Ad 1") self.assertContains(resp, "Create fallback ad") - member.role = "Reporter" + member.role = UserPublisherMember.ROLE_REPORTER member.save() resp = self.client.get(url) @@ -637,7 +637,7 @@ def test_fallback_ads_detail(self): member = UserPublisherMember.objects.create( user=self.user, publisher=self.publisher, - role="Manager", + role=UserPublisherMember.ROLE_MANAGER, ) resp = self.client.get(url) @@ -645,7 +645,7 @@ def test_fallback_ads_detail(self): self.assertContains(resp, "Test Ad 1") self.assertContains(resp, "Edit fallback ad") - member.role = "Reporter" + member.role = UserPublisherMember.ROLE_REPORTER member.save() resp = self.client.get(url) @@ -671,14 +671,14 @@ def test_fallback_ads_update(self): member = UserPublisherMember.objects.create( user=self.user, publisher=self.publisher, - role="Reporter", + role=UserPublisherMember.ROLE_REPORTER, ) # Reporters can't edit resp = self.client.get(url) self.assertEqual(resp.status_code, 403) - member.role = "Manager" + member.role = UserPublisherMember.ROLE_MANAGER member.save() resp = self.client.get(url) @@ -719,14 +719,14 @@ def test_fallback_ads_create(self): member = UserPublisherMember.objects.create( user=self.user, publisher=self.publisher, - role="Reporter", + role=UserPublisherMember.ROLE_REPORTER, ) # Reporter can't create resp = self.client.get(url) self.assertEqual(resp.status_code, 403) - member.role = "Manager" + member.role = UserPublisherMember.ROLE_MANAGER member.save() resp = self.client.get(url) From c9c45f169f00379aaa4e104c05bff488449f9910 Mon Sep 17 00:00:00 2001 From: David Fischer Date: Mon, 18 Nov 2024 16:55:48 -0800 Subject: [PATCH 3/3] Feedback updates --- adserver/auth/models.py | 82 +++++++++++++++++++ adserver/mixins.py | 34 ++++---- .../advertiser/advertisement-detail.html | 7 +- .../adserver/advertiser/flight-detail.html | 15 ++-- .../adserver/advertiser/flight-list.html | 7 +- .../templates/adserver/advertiser/users.html | 11 +-- adserver/templates/adserver/base.html | 4 +- .../publisher/fallback-ads-detail.html | 7 +- .../adserver/publisher/fallback-ads-list.html | 12 ++- .../templates/adserver/publisher/users.html | 9 +- adserver/templatetags/ad_extras.py | 59 ++++++------- adserver/tests/test_advertiser_dashboard.py | 27 ++++++ 12 files changed, 185 insertions(+), 89 deletions(-) diff --git a/adserver/auth/models.py b/adserver/auth/models.py index cea8c166..0fe9ec92 100644 --- a/adserver/auth/models.py +++ b/adserver/auth/models.py @@ -119,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( @@ -135,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) @@ -173,6 +251,8 @@ class UserAdvertiserMember(models.Model): ) 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" @@ -197,4 +277,6 @@ class UserPublisherMember(models.Model): ) 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/mixins.py b/adserver/mixins.py index 87116da5..202352a9 100644 --- a/adserver/mixins.py +++ b/adserver/mixins.py @@ -41,18 +41,21 @@ class AdvertiserAccessMixin: 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 - return ( - self.request.user.is_staff - or self.request.user.useradvertisermember_set.filter( - advertiser__slug=self.kwargs[self.advertiser_slug_parameter], - role__in=self.allowed_roles, - ).exists() - ) + 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): @@ -80,18 +83,21 @@ class PublisherAccessMixin: 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 - return ( - self.request.user.is_staff - or self.request.user.userpublishermember_set.filter( - publisher__slug=self.kwargs[self.publisher_slug_parameter], - role__in=self.allowed_roles, - ).exists() - ) + 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): diff --git a/adserver/templates/adserver/advertiser/advertisement-detail.html b/adserver/templates/adserver/advertiser/advertisement-detail.html index 8cb50ab0..6685be0d 100644 --- a/adserver/templates/adserver/advertiser/advertisement-detail.html +++ b/adserver/templates/adserver/advertiser/advertisement-detail.html @@ -5,9 +5,6 @@ {% load ad_extras %} -{% advertiser_role request.user advertiser as user_advertiser_role %} - - {% block title %}{% trans 'Advertisement: ' %}{{ advertisement.name }}{% endblock %} @@ -21,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 %} @@ -65,7 +64,7 @@

{% block heading %}{% trans 'Advertisement: ' %}{{ advertisement.name }}{% e
- {% if request.user.is_staff or user_advertiser_role == "Admin" or user_advertiser_role == "Manager" %} + {% if has_advertiser_edit_permission %}
{% trans 'Edit advertisement' %}
diff --git a/adserver/templates/adserver/advertiser/flight-detail.html b/adserver/templates/adserver/advertiser/flight-detail.html index 7364cb8d..f1467aa0 100644 --- a/adserver/templates/adserver/advertiser/flight-detail.html +++ b/adserver/templates/adserver/advertiser/flight-detail.html @@ -5,9 +5,6 @@ {% load ad_extras %} -{% advertiser_role request.user advertiser as user_advertiser_role %} - - {% block title %}{{ flight.name }}{% endblock %} @@ -20,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 %}

@@ -34,7 +33,7 @@

{% block heading %}{% trans 'Flight' %}: {{ flight.name }}{% endblock headin {% trans 'See full report' %} - {% if request.user.is_staff or user_advertiser_role == "Admin" or user_advertiser_role == "Manager" %} + {% if has_advertiser_edit_permission %} {% if flight.auto_renew %}{% trans 'Renewing' %}{% else %}{% trans 'Automatically renew' %}{% endif %} @@ -60,7 +59,7 @@

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

{% trans 'Live ads' %}

 {% endfor %} diff --git a/adserver/templates/adserver/base.html b/adserver/templates/adserver/base.html index 68d7057e..f303b8f4 100644 --- a/adserver/templates/adserver/base.html +++ b/adserver/templates/adserver/base.html @@ -144,7 +144,7 @@
{{ advertiser }}
{% elif publisher %} - {% publisher_role request.user publisher as user_publisher_role %} + {% 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 request.user.is_staff or user_publisher_role == "Admin" %} + {% if request.user.id != member.user.id %} + {% trans 'remove' %} + {% endif %} {% endif %}
{{ member.role }} - {% if request.user.is_staff or user_advertiser_role == "Admin" %} - {% if request.user.id != member.user.id %} + {% if has_advertiser_admin_permission and request.user.id != member.user.id %} {% trans 'remove' %} {% endif %} - {% endif %}
{{ member.user.email }} {{ member.role }} - {% if request.user.is_staff or user_publisher_role == "Admin" %} + {% if has_publisher_admin_permission %} {% if request.user.id != member.user.id %} {% trans 'remove' %} {% endif %} diff --git a/adserver/templatetags/ad_extras.py b/adserver/templatetags/ad_extras.py index f2fa7690..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() @@ -17,52 +20,40 @@ def advertisement_preview(ad, ad_type=None): @register.simple_tag -def advertiser_role(user, advertiser): +def advertiser_manager_role(user, advertiser): """ - Returns the users role in this advertiser or None if the user has no permissions. + Returns True if the user has manager or higher role on this advertiser and False otherwise. - Staff status is not taken into account. Caches the result on the user so future calls - don't involve a DB lookup. + Return True for staff. """ - if not hasattr(user, "_advertiser_roles"): - user._advertiser_roles = {} - - if advertiser.pk in user._advertiser_roles: - return user._advertiser_roles[advertiser.pk] + return user.is_staff or user.has_advertiser_manager_permission(advertiser) - membership = user.useradvertisermember_set.filter( - advertiser=advertiser, - ).first() - role = None - if membership: - role = membership.role +@register.simple_tag +def advertiser_admin_role(user, advertiser): + """ + Returns True if the user has admin role on this advertiser and False otherwise. - user._advertiser_roles[advertiser.pk] = role - return role + Return True for staff. + """ + return user.is_staff or user.has_advertiser_admin_permission(advertiser) @register.simple_tag -def publisher_role(user, publisher): +def publisher_manager_role(user, publisher): """ - Returns the users role in this publisher or None if the user has no permissions. + Returns True if the user has manager or higher role on this publisher and False otherwise. - Staff status is not taken into account. Caches the result on the user so future calls - don't involve a DB lookup. + Return True for staff. """ - if not hasattr(user, "_publisher_roles"): - user._publisher_roles = {} + return user.is_staff or user.has_publisher_manager_permission(publisher) - if publisher.pk in user._publisher_roles: - return user._publisher_roles[publisher.pk] - membership = user.userpublishermember_set.filter( - publisher=publisher, - ).first() - - role = None - if membership: - role = membership.role +@register.simple_tag +def publisher_admin_role(user, publisher): + """ + Returns True if the user has admin role on this publisher and False otherwise. - user._publisher_roles[publisher.pk] = role - return role + 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 55de382e..ecaf6e4d 100644 --- a/adserver/tests/test_advertiser_dashboard.py +++ b/adserver/tests/test_advertiser_dashboard.py @@ -204,6 +204,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", @@ -234,6 +243,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", @@ -707,6 +725,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",