From b6614186b3c742836c7fc9b255d1c8c40285acb0 Mon Sep 17 00:00:00 2001 From: Tasos Katsoulas Date: Tue, 11 Aug 2026 11:21:57 +0300 Subject: [PATCH] Make NDA functionality more robust * Default employees to all events * Make NDA option inclusive for Community * Fix a bug in upvoting for NDA events --- moderator/moderate/admin.py | 8 +- moderator/moderate/auth.py | 47 +++- moderator/moderate/forms.py | 21 +- .../0023_mozillianprofile_is_employee.py | 18 ++ .../migrations/0024_backfill_is_employee.py | 47 ++++ ...5_remove_mozillianprofile_is_nda_member.py | 18 ++ moderator/moderate/models.py | 21 +- moderator/moderate/moderate_urls.py | 2 +- .../moderate/templates/create_event.jinja | 3 +- moderator/moderate/templates/questions.jinja | 2 +- moderator/moderate/templatetags/helpers.py | 12 + moderator/moderate/tests/conftest.py | 22 ++ moderator/moderate/tests/test_auth.py | 69 +++++- moderator/moderate/tests/test_forms.py | 71 +++++- moderator/moderate/tests/test_models.py | 51 +++- moderator/moderate/tests/test_urls.py | 223 +++++++++++++++++- moderator/moderate/tests/test_utils.py | 41 ++++ moderator/moderate/utils.py | 12 +- moderator/moderate/views.py | 39 ++- moderator/settings.py | 11 +- setup.cfg | 2 + 21 files changed, 675 insertions(+), 65 deletions(-) create mode 100644 moderator/moderate/migrations/0023_mozillianprofile_is_employee.py create mode 100644 moderator/moderate/migrations/0024_backfill_is_employee.py create mode 100644 moderator/moderate/migrations/0025_remove_mozillianprofile_is_nda_member.py diff --git a/moderator/moderate/admin.py b/moderator/moderate/admin.py index 33f69ea..f500f86 100644 --- a/moderator/moderate/admin.py +++ b/moderator/moderate/admin.py @@ -43,15 +43,15 @@ class UserAdmin(UserAdmin): "email", "first_name", "last_name", - "is_nda_member", + "is_employee", "is_staff", ) search_fields = ["email", "first_name", "last_name"] - def is_nda_member(self, obj): - return obj.userprofile.is_nda_member + def is_employee(self, obj): + return obj.userprofile.is_employee - is_nda_member.boolean = True + is_employee.boolean = True class QuestionInline(admin.StackedInline): diff --git a/moderator/moderate/auth.py b/moderator/moderate/auth.py index cb9ab41..0219712 100644 --- a/moderator/moderate/auth.py +++ b/moderator/moderate/auth.py @@ -1,18 +1,40 @@ from django.conf import settings from mozilla_django_oidc.auth import OIDCAuthenticationBackend -from moderator.moderate.utils import is_legacy_username, suggest_username +from moderator.moderate.utils import ( + is_employee_groups, + is_legacy_username, + suggest_username, +) + +GROUPS_CLAIM = "https://sso.mozilla.com/claim/groups" class ModeratorAuthBackend(OIDCAuthenticationBackend): """Override base authentication class.""" + _userinfo = None + _userinfo_access_token = None + + def get_userinfo(self, access_token, id_token, payload): + """Fetch the claims once per login. + + `get_or_create_user` needs them to gate the login and the base + implementation asks the provider for them again right afterwards. + """ + if self._userinfo is None or access_token != self._userinfo_access_token: + self._userinfo = super(ModeratorAuthBackend, self).get_userinfo( + access_token, id_token, payload + ) + self._userinfo_access_token = access_token + return self._userinfo + def get_or_create_user(self, access_token, id_token, payload): """Get or create a new user only if they have one of the groups mentioned in the ALLOWED_LOGIN_GROUPS in the claims. """ user_info = self.get_userinfo(access_token, id_token, payload) - groups = user_info.get("https://sso.mozilla.com/claim/groups", []) + groups = user_info.get(GROUPS_CLAIM, []) # The user is not staff or NDA member. Return None if not any(x in groups for x in settings.ALLOWED_LOGIN_GROUPS): @@ -21,21 +43,24 @@ def get_or_create_user(self, access_token, id_token, payload): access_token, id_token, payload ) + def create_user(self, claims): + user = super(ModeratorAuthBackend, self).create_user(claims) + self.update_profile(user, claims) + return user + def update_user(self, user, claims): - # Update user status (nda, staff based on assertions) - profile = user.userprofile email = claims.get("email") if email and user.email != email: user.email = email if is_legacy_username(user.username): user.username = suggest_username(user.email) - profile.avatar_url = claims.get("avatar", "") user.save() + self.update_profile(user, claims) + return user - # Only staff members and members of the NDA group are allowed to login. - # Because of this everyone will get the is_nda_member set to True. - # If in the future more people are allowed to login this needs to be - # available to only members of the ALLOWED_LOGIN_GROUPS - profile.is_nda_member = True + def update_profile(self, user, claims): + """Refresh the profile fields derived from the OIDC claims.""" + profile = user.userprofile + profile.avatar_url = claims.get("avatar", "") + profile.is_employee = is_employee_groups(claims.get(GROUPS_CLAIM, [])) profile.save() - return user diff --git a/moderator/moderate/forms.py b/moderator/moderate/forms.py index 6475107..aed950e 100644 --- a/moderator/moderate/forms.py +++ b/moderator/moderate/forms.py @@ -16,7 +16,9 @@ class TomSelectMultiple(forms.SelectMultiple): def __init__(self, autocomplete_url, attrs=None): merged = { "data-autocomplete-url": autocomplete_url, - "class": ((attrs or {}).get("class", "") + " tom-select form-control").strip(), + "class": ( + (attrs or {}).get("class", "") + " tom-select form-control" + ).strip(), } merged.update({k: v for k, v in (attrs or {}).items() if k not in merged}) super().__init__(attrs=merged) @@ -134,17 +136,18 @@ def __init__(self, *args, **kwargs): else: self.fields["moderators"].initial = User.objects.filter(id=self.user.pk) del self.fields["archived"] + if not self.user.userprofile.is_employee: + # An NDA community member would lose sight of their own event if it + # were not opted in to the NDA community. + self.fields["is_nda"].disabled = True + self.fields["is_nda"].initial = True + self.fields["is_nda"].help_text = ( + "Only staff can change who an event is open to." + ) def clean(self): - """ - Clean method to check post data for nda events, - and moderated events with no moderators. - """ + """Clean method to check for moderated events with no moderators.""" cdata = super(EventForm, self).clean() - # Do not allow non-nda members to submit NDA events. - if not self.user.userprofile.is_nda_member and cdata["is_nda"]: - msg = "Only members of the NDA group can create NDA events." - raise forms.ValidationError(msg) # Don't allow non-superusers to modify moderation status or moderators if not cdata["moderators"]: msg = "An event should have at least one moderator." diff --git a/moderator/moderate/migrations/0023_mozillianprofile_is_employee.py b/moderator/moderate/migrations/0023_mozillianprofile_is_employee.py new file mode 100644 index 0000000..7f52fa3 --- /dev/null +++ b/moderator/moderate/migrations/0023_mozillianprofile_is_employee.py @@ -0,0 +1,18 @@ +# Generated by Django 5.2.13 on 2026-08-04 09:19 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("moderate", "0022_alter_event_options"), + ] + + operations = [ + migrations.AddField( + model_name="mozillianprofile", + name="is_employee", + field=models.BooleanField(default=False), + ), + ] diff --git a/moderator/moderate/migrations/0024_backfill_is_employee.py b/moderator/moderate/migrations/0024_backfill_is_employee.py new file mode 100644 index 0000000..d3649e9 --- /dev/null +++ b/moderator/moderate/migrations/0024_backfill_is_employee.py @@ -0,0 +1,47 @@ +from django.db import migrations + +# Kept separate from the schema migration: MySQL cannot roll DDL back, so a +# failure here would otherwise leave the column added and the migration +# unrecorded. + +# Staff domains that do not carry the "mozilla" substring. +EMPLOYEE_EMAIL_DOMAINS = frozenset({"thunderbird.net", "getpocket.com"}) + + +def is_employee_email(email): + """True if `email` looks like a staff address. + + Deliberately duplicated here instead of imported from the app: migrations + have to keep working when application code moves on. + """ + if not email: + return False + domain = email.rsplit("@", 1)[-1].lower() + return "mozilla" in domain or domain in EMPLOYEE_EMAIL_DOMAINS + + +def backfill_is_employee(apps, schema_editor): + """Classify existing profiles by email domain. + + Nothing recorded whether a profile belonged to staff before this migration, + so seed the flag from the address and let the OIDC claim correct it on each + user's next login. + """ + MozillianProfile = apps.get_model("moderate", "MozillianProfile") + employees = [] + for profile in MozillianProfile.objects.select_related("user").iterator(): + if is_employee_email(profile.user.email): + profile.is_employee = True + employees.append(profile) + MozillianProfile.objects.bulk_update(employees, ["is_employee"], batch_size=500) + + +class Migration(migrations.Migration): + + dependencies = [ + ("moderate", "0023_mozillianprofile_is_employee"), + ] + + operations = [ + migrations.RunPython(backfill_is_employee, migrations.RunPython.noop), + ] diff --git a/moderator/moderate/migrations/0025_remove_mozillianprofile_is_nda_member.py b/moderator/moderate/migrations/0025_remove_mozillianprofile_is_nda_member.py new file mode 100644 index 0000000..e397ff7 --- /dev/null +++ b/moderator/moderate/migrations/0025_remove_mozillianprofile_is_nda_member.py @@ -0,0 +1,18 @@ +from django.db import migrations + +# Dropping the old column is split out so it can be applied on a later deploy, +# once no instance is running code that still reads is_nda_member. + + +class Migration(migrations.Migration): + + dependencies = [ + ("moderate", "0024_backfill_is_employee"), + ] + + operations = [ + migrations.RemoveField( + model_name="mozillianprofile", + name="is_nda_member", + ), + ] diff --git a/moderator/moderate/models.py b/moderator/moderate/models.py index c445862..aaeb0c8 100644 --- a/moderator/moderate/models.py +++ b/moderator/moderate/models.py @@ -23,7 +23,7 @@ class MozillianProfile(models.Model): slug = models.SlugField(blank=True, max_length=100) username = models.CharField(max_length=40) avatar_url = models.URLField(max_length=400, default="", blank=True) - is_nda_member = models.BooleanField(default=False) + is_employee = models.BooleanField(default=False) def __str__(self): return self.username @@ -60,6 +60,23 @@ def create_user_profile(sender, instance, created, raw, **kwargs): ) +class EventQuerySet(models.QuerySet): + def visible_to(self, user): + """Restrict to the events `user` is allowed to see. + + Employees and superusers see everything. NDA community members see the + events opted in to the NDA community plus the ones they moderate, so a + community moderator can still find their own event. + """ + if user.is_superuser or user.userprofile.is_employee: + return self + # A subquery rather than a join on moderators: joining would duplicate + # rows and inflate the Count() annotations the listing views add. + return self.filter( + models.Q(is_nda=True) | models.Q(pk__in=user.events_moderated.values("pk")) + ) + + class Event(models.Model): """Event model.""" @@ -82,6 +99,8 @@ class Event(models.Model): is_moderated = models.BooleanField(default=False) users_can_vote = models.BooleanField(default=True) + objects = EventQuerySet.as_manager() + class Meta: ordering = ["-event_date"] diff --git a/moderator/moderate/moderate_urls.py b/moderator/moderate/moderate_urls.py index 945f573..de88e41 100644 --- a/moderator/moderate/moderate_urls.py +++ b/moderator/moderate/moderate_urls.py @@ -35,7 +35,7 @@ ] question_urls = [ - path("/upvote", moderate_views.upvote, name="upvote"), + path("/upvote", moderate_views.upvote, name="upvote"), ] urlpatterns = [ diff --git a/moderator/moderate/templates/create_event.jinja b/moderator/moderate/templates/create_event.jinja index 720f638..291b0ad 100644 --- a/moderator/moderate/templates/create_event.jinja +++ b/moderator/moderate/templates/create_event.jinja @@ -47,7 +47,8 @@ {% if user_can_edit %} {{ check(event_form.users_can_vote, "Allow users to vote on questions") }} {{ check(event_form.is_nda, - 'Restrict to NDA Community members') }} + 'Allow NDA Community members', + help=event_form.is_nda.help_text) }} {{ check(event_form.is_moderated, "Moderated event (questions need approval)") }} {% if event %} {{ check(event_form.archived, "Archive this event") }} diff --git a/moderator/moderate/templates/questions.jinja b/moderator/moderate/templates/questions.jinja index 99408a8..b9708b4 100644 --- a/moderator/moderate/templates/questions.jinja +++ b/moderator/moderate/templates/questions.jinja @@ -25,7 +25,7 @@ {{ event.event_date|date('F j, Y') }} {% endif %} - {% if event.is_nda %}NDA{% endif %} + {% if event.is_nda %}NDA Community{% endif %} {% if event.is_moderated %}Moderated{% endif %} {% if event.archived %}Archived{% endif %} diff --git a/moderator/moderate/templatetags/helpers.py b/moderator/moderate/templatetags/helpers.py index 20494f0..91ff187 100644 --- a/moderator/moderate/templatetags/helpers.py +++ b/moderator/moderate/templatetags/helpers.py @@ -4,6 +4,8 @@ from django.utils.safestring import mark_safe from django_jinja import library +from moderator.moderate.models import Event + @library.global_function def user_voted(question, user): @@ -24,6 +26,16 @@ def can_moderate_event(event, user): return user.is_superuser or event.moderators.filter(id=user.id).exists() +@library.global_function +def can_access_event(event, user): + """Check if a user can see an event. + + Delegates to the queryset so the object level and the list level rules + cannot drift apart. + """ + return Event.objects.filter(pk=event.pk).visible_to(user).exists() + + @library.global_function def can_answer_question(question, user): """Check if a user can answer a question.""" diff --git a/moderator/moderate/tests/conftest.py b/moderator/moderate/tests/conftest.py index 22c2841..21aa0a4 100644 --- a/moderator/moderate/tests/conftest.py +++ b/moderator/moderate/tests/conftest.py @@ -1,6 +1,28 @@ +import pytest + + def pytest_configure(config): from django.conf import settings settings.SESSION_COOKIE_SECURE = False settings.CSRF_COOKIE_SECURE = False settings.SECURE_HSTS_SECONDS = 0 + + +@pytest.fixture +def make_user(db): + """Build a user whose profile is flagged as staff or NDA community.""" + + def _make_user(username, is_employee=False, superuser=False): + from django.contrib.auth.models import User + + create = ( + User.objects.create_superuser if superuser else User.objects.create_user + ) + user = create(username=username, email=f"{username}@example.com", password="x") + profile = user.userprofile + profile.is_employee = is_employee + profile.save() + return user + + return _make_user diff --git a/moderator/moderate/tests/test_auth.py b/moderator/moderate/tests/test_auth.py index 400a5cd..0fbe5bd 100644 --- a/moderator/moderate/tests/test_auth.py +++ b/moderator/moderate/tests/test_auth.py @@ -1,7 +1,28 @@ import pytest from django.contrib.auth.models import User +from mozilla_django_oidc.auth import OIDCAuthenticationBackend -from moderator.moderate.auth import ModeratorAuthBackend +from moderator.moderate.auth import GROUPS_CLAIM, ModeratorAuthBackend + + +def test_get_userinfo_asks_the_provider_once_per_access_token(monkeypatch): + """get_or_create_user and the base implementation share one lookup.""" + calls = [] + + def fake_get_userinfo(self, access_token, id_token, payload): + calls.append(access_token) + return {"email": "jane@mozilla.com", GROUPS_CLAIM: ["team_moco"]} + + monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", fake_get_userinfo) + backend = ModeratorAuthBackend() + + assert backend.get_userinfo("token", None, {}) == backend.get_userinfo( + "token", None, {} + ) + assert calls == ["token"] + + backend.get_userinfo("other-token", None, {}) + assert calls == ["token", "other-token"] @pytest.mark.django_db @@ -45,6 +66,52 @@ def test_update_user_backfill_resolves_collision(): assert user.username == "jane1" +@pytest.mark.django_db +def test_create_user_flags_employee_on_first_login(): + """First login goes through create_user, not update_user.""" + backend = ModeratorAuthBackend() + user = backend.create_user( + {"email": "newperson@mozilla.com", GROUPS_CLAIM: ["team_moco"]} + ) + assert user.userprofile.is_employee is True + + +@pytest.mark.django_db +def test_create_user_does_not_flag_community_member(): + backend = ModeratorAuthBackend() + user = backend.create_user( + {"email": "contributor@example.com", GROUPS_CLAIM: ["mozilliansorg_nda"]} + ) + assert user.userprofile.is_employee is False + + +@pytest.mark.django_db +def test_update_user_flags_employee(): + user = User.objects.create_user(username="jane", email="jane@mozilla.com") + backend = ModeratorAuthBackend() + backend.update_user( + user, {"email": "jane@mozilla.com", GROUPS_CLAIM: ["team_mzla"]} + ) + user.userprofile.refresh_from_db() + assert user.userprofile.is_employee is True + + +@pytest.mark.django_db +def test_update_user_clears_employee_flag_when_claim_drops_staff_group(): + """A contributor who left staff must lose access to employee only events.""" + user = User.objects.create_user(username="jane", email="jane@mozilla.com") + profile = user.userprofile + profile.is_employee = True + profile.save() + + backend = ModeratorAuthBackend() + backend.update_user( + user, {"email": "jane@example.com", GROUPS_CLAIM: ["mozilliansorg_nda"]} + ) + profile.refresh_from_db() + assert profile.is_employee is False + + @pytest.mark.django_db def test_update_user_updates_email_before_deriving_username(): # User's email in claims differs from stored email; the derivation diff --git a/moderator/moderate/tests/test_forms.py b/moderator/moderate/tests/test_forms.py index 7250e5b..abc5914 100644 --- a/moderator/moderate/tests/test_forms.py +++ b/moderator/moderate/tests/test_forms.py @@ -1,6 +1,13 @@ import pytest -from moderator.moderate.forms import QuestionForm +from moderator.moderate.forms import EventForm, QuestionForm +from moderator.moderate.models import Event + + +def _event_data(user, **overrides): + data = {"name": "Test event", "body": "", "moderators": [user.pk]} + data.update(overrides) + return data @pytest.mark.django_db @@ -21,3 +28,65 @@ def test_question_form_rejects_too_long_text(): def test_question_form_accepts_valid_text(): form = QuestionForm(data={"question": "This is a perfectly valid question."}) assert form.is_valid(), form.errors + + +@pytest.mark.django_db +def test_event_form_forces_nda_for_community_member(make_user): + """A community member would lose sight of an event that is not opted in.""" + user = make_user("contributor") + form = EventForm(_event_data(user, is_nda=False), user=user) + assert form.is_valid(), form.errors + assert form.save().is_nda is True + + +@pytest.mark.django_db +def test_event_form_forces_nda_when_community_member_omits_the_field(make_user): + user = make_user("contributor") + form = EventForm(_event_data(user), user=user) + assert form.is_valid(), form.errors + assert form.save().is_nda is True + + +@pytest.mark.django_db +def test_event_form_lets_employee_opt_out(make_user): + user = make_user("staff", is_employee=True) + form = EventForm(_event_data(user, is_nda=False), user=user) + assert form.is_valid(), form.errors + assert form.save().is_nda is False + + +@pytest.mark.django_db +def test_event_form_lets_employee_opt_in(make_user): + user = make_user("staff", is_employee=True) + form = EventForm(_event_data(user, is_nda=True), user=user) + assert form.is_valid(), form.errors + assert form.save().is_nda is True + + +@pytest.mark.django_db +def test_event_form_community_moderator_cannot_change_existing_value(make_user): + staff = make_user("staff", is_employee=True) + contributor = make_user("contributor") + event = Event.objects.create(name="Staff only", is_nda=False, created_by=staff) + event.moderators.set([staff, contributor]) + + form = EventForm( + _event_data( + contributor, + name="Staff only", + moderators=[staff.pk, contributor.pk], + is_nda=True, + ), + instance=event, + user=contributor, + ) + assert form.is_valid(), form.errors + assert form.save().is_nda is False + + +@pytest.mark.django_db +def test_event_form_no_longer_blocks_community_member_from_nda_events(make_user): + """The old "only NDA members can create NDA events" rule is gone.""" + user = make_user("contributor") + form = EventForm(_event_data(user, is_nda=True), user=user) + assert form.is_valid(), form.errors diff --git a/moderator/moderate/tests/test_models.py b/moderator/moderate/tests/test_models.py index d7cd661..70c0bc2 100644 --- a/moderator/moderate/tests/test_models.py +++ b/moderator/moderate/tests/test_models.py @@ -1,6 +1,7 @@ import pytest from django.contrib.auth.models import User from django.core.exceptions import ValidationError +from django.db.models import Count, Q from moderator.moderate.models import Event, MozillianProfile, Question, Vote @@ -42,9 +43,7 @@ def test_question_full_clean_rejects_short_question(): def test_question_full_clean_accepts_valid_length(): user = User.objects.create_user(username="d", email="d@example.com") event = Event.objects.create(name="E", created_by=user) - q = Question( - asked_by=user, event=event, question="This is a valid question." - ) + q = Question(asked_by=user, event=event, question="This is a valid question.") q.full_clean() @@ -73,3 +72,49 @@ def test_event_questions_count_property(): asked_by=user, event=event, question="Second valid question body." ) assert event.questions_count == 2 + + +@pytest.mark.django_db +def test_visible_to_returns_opted_in_and_moderated_events(make_user): + contributor = make_user("contributor") + community = Event.objects.create(name="Community", is_nda=True) + moderated = Event.objects.create(name="Moderated", is_nda=False) + moderated.moderators.set([contributor]) + Event.objects.create(name="Staff Only", is_nda=False) + + visible = Event.objects.visible_to(contributor) + assert set(visible.values_list("name", flat=True)) == {"Community", "Moderated"} + assert community in visible + + +@pytest.mark.django_db +def test_visible_to_returns_everything_for_employees_and_superusers(make_user): + Event.objects.create(name="Staff Only", is_nda=False) + Event.objects.create(name="Community", is_nda=True) + + for user in [ + make_user("staff", is_employee=True), + make_user("root", superuser=True), + ]: + assert Event.objects.visible_to(user).count() == 2 + + +@pytest.mark.django_db +def test_visible_to_does_not_inflate_annotations(make_user): + """The moderator clause must not join and double the Count() annotations.""" + contributor = make_user("contributor") + event = Event.objects.create(name="Staff Only", is_nda=False) + event.moderators.set([contributor, make_user("staff", is_employee=True)]) + for i in range(3): + Question.objects.create( + event=event, question=f"Question number {i} body text.", is_accepted=True + ) + + annotated = ( + Event.objects.visible_to(contributor) + .annotate( + approved_count=Count("questions", filter=Q(questions__is_accepted=True)) + ) + .get() + ) + assert annotated.approved_count == 3 diff --git a/moderator/moderate/tests/test_urls.py b/moderator/moderate/tests/test_urls.py index bd5b10b..26c753a 100644 --- a/moderator/moderate/tests/test_urls.py +++ b/moderator/moderate/tests/test_urls.py @@ -5,7 +5,7 @@ from django.test import Client from django.utils.timezone import now as django_now -from moderator.moderate.models import Event +from moderator.moderate.models import Event, Question @pytest.mark.django_db @@ -41,14 +41,31 @@ def test_authenticated_user_can_create_event(): assert resp.status_code == 200 +@pytest.mark.django_db +def test_create_event_page_explains_locked_nda_choice(make_user): + """A community member sees the option, disabled, rather than nothing.""" + client = Client() + client.force_login(make_user("contributor")) + resp = client.get("/event/new") + assert b"NDA Community members" in resp.content + assert b"Only staff can change who an event is open to." in resp.content + + +@pytest.mark.django_db +def test_create_event_page_leaves_nda_choice_open_to_employee(make_user): + client = Client() + client.force_login(make_user("staff", is_employee=True)) + resp = client.get("/event/new") + assert b"NDA Community members" in resp.content + assert b"Only staff can change who an event is open to." not in resp.content + + @pytest.mark.django_db def test_user_autocomplete_returns_matches(): user = User.objects.create_user( username="alice", email="alice@example.com", password="x" ) - User.objects.create_user( - username="bob", email="bob@example.com", password="x" - ) + User.objects.create_user(username="bob", email="bob@example.com", password="x") client = Client() client.force_login(user) resp = client.get("/u/user-autocomplete/?q=alic") @@ -139,6 +156,204 @@ def test_archive_event_rejects_future_event(): assert event.archived is False +def _make_events(archived=False): + staff_only = Event.objects.create( + name="Staff Only Event", is_nda=False, archived=archived + ) + community = Event.objects.create( + name="Community Welcome Event", is_nda=True, archived=archived + ) + return staff_only, community + + +@pytest.mark.django_db +def test_index_hides_staff_only_events_from_community_member(make_user): + _make_events() + client = Client() + client.force_login(make_user("contributor")) + resp = client.get("/") + assert b"Community Welcome Event" in resp.content + assert b"Staff Only Event" not in resp.content + + +@pytest.mark.django_db +def test_index_shows_every_event_to_employee(make_user): + _make_events() + client = Client() + client.force_login(make_user("staff", is_employee=True)) + resp = client.get("/") + assert b"Community Welcome Event" in resp.content + assert b"Staff Only Event" in resp.content + + +@pytest.mark.django_db +def test_archive_hides_staff_only_events_from_community_member(make_user): + _make_events(archived=True) + client = Client() + client.force_login(make_user("contributor")) + resp = client.get("/archives") + assert b"Community Welcome Event" in resp.content + assert b"Staff Only Event" not in resp.content + + +@pytest.mark.django_db +def test_archive_shows_every_event_to_employee(make_user): + _make_events(archived=True) + client = Client() + client.force_login(make_user("staff", is_employee=True)) + resp = client.get("/archives") + assert b"Community Welcome Event" in resp.content + assert b"Staff Only Event" in resp.content + + +@pytest.mark.django_db +def test_event_page_404s_for_community_member_on_staff_only_event(make_user): + staff_only, _ = _make_events() + client = Client() + client.force_login(make_user("contributor")) + assert client.get(f"/e/{staff_only.slug}/").status_code == 404 + + +@pytest.mark.django_db +def test_event_page_open_to_community_member_when_opted_in(make_user): + _, community = _make_events() + client = Client() + client.force_login(make_user("contributor")) + assert client.get(f"/e/{community.slug}/").status_code == 200 + + +@pytest.mark.django_db +def test_event_page_open_to_employee_either_way(make_user): + staff_only, community = _make_events() + client = Client() + client.force_login(make_user("staff", is_employee=True)) + assert client.get(f"/e/{staff_only.slug}/").status_code == 200 + assert client.get(f"/e/{community.slug}/").status_code == 200 + + +@pytest.mark.django_db +def test_event_page_open_to_community_moderator_of_staff_only_event(make_user): + """Moderators keep access so the moderation queue stays reachable.""" + staff_only, _ = _make_events() + contributor = make_user("contributor") + staff_only.moderators.set([contributor]) + client = Client() + client.force_login(contributor) + assert client.get(f"/e/{staff_only.slug}/").status_code == 200 + + +@pytest.mark.django_db +def test_index_lists_staff_only_event_a_community_member_moderates(make_user): + """An event the user can open has to be reachable from the listing too.""" + staff_only, _ = _make_events() + contributor = make_user("contributor") + staff_only.moderators.set([contributor]) + client = Client() + client.force_login(contributor) + assert b"Staff Only Event" in client.get("/").content + + +@pytest.mark.django_db +def test_archive_lists_staff_only_event_a_community_member_moderates(make_user): + staff_only, _ = _make_events(archived=True) + contributor = make_user("contributor") + staff_only.moderators.set([contributor]) + client = Client() + client.force_login(contributor) + assert b"Staff Only Event" in client.get("/archives").content + + +@pytest.mark.django_db +def test_index_shows_every_event_to_superuser_without_employee_flag(make_user): + """The email based backfill can leave an admin account unflagged.""" + _make_events() + client = Client() + client.force_login(make_user("root", superuser=True)) + resp = client.get("/") + assert b"Community Welcome Event" in resp.content + assert b"Staff Only Event" in resp.content + + +@pytest.mark.django_db +def test_reply_url_404s_for_question_from_another_event(make_user): + """The question has to belong to the event in the URL.""" + staff_only, community = _make_events() + secret = Question.objects.create( + event=staff_only, question="Secret staff only question.", is_accepted=True + ) + client = Client() + client.force_login(make_user("contributor")) + resp = client.get(f"/e/{community.slug}/q/{secret.id}/reply") + assert resp.status_code == 404 + assert b"Secret staff only question." not in resp.content + + +@pytest.mark.django_db +def test_moderate_url_404s_for_question_from_another_event(make_user): + """A moderator of one event must not moderate another event's questions.""" + staff_only, community = _make_events() + contributor = make_user("contributor") + community.moderators.set([contributor]) + other = Question.objects.create( + event=staff_only, question="A question with enough text.", is_accepted=None + ) + client = Client() + client.force_login(contributor) + resp = client.get(f"/e/{community.slug}/moderate/{other.id}/accepted") + assert resp.status_code == 404 + other.refresh_from_db() + assert other.is_accepted is None + + +@pytest.mark.django_db +def test_upvote_404s_for_community_member_on_staff_only_event(make_user): + staff_only, _ = _make_events() + question = Question.objects.create( + event=staff_only, question="A question with enough text.", is_accepted=True + ) + client = Client() + client.force_login(make_user("contributor")) + resp = client.post( + f"/q/{question.id}/upvote", headers={"x-requested-with": "XMLHttpRequest"} + ) + assert resp.status_code == 404 + assert question.votes.count() == 0 + + +@pytest.mark.django_db +def test_upvote_404s_for_unknown_question(make_user): + client = Client() + client.force_login(make_user("staff", is_employee=True)) + resp = client.post("/q/4711/upvote", headers={"x-requested-with": "XMLHttpRequest"}) + assert resp.status_code == 404 + + +@pytest.mark.django_db +def test_upvote_404s_for_non_numeric_question_id(make_user): + client = Client() + client.force_login(make_user("staff", is_employee=True)) + resp = client.post( + "/q/not-a-number/upvote", headers={"x-requested-with": "XMLHttpRequest"} + ) + assert resp.status_code == 404 + + +@pytest.mark.django_db +def test_upvote_respects_voting_switch_on_nda_event(make_user): + """Opting an event in to the NDA community must not re-enable voting.""" + event = Event.objects.create(name="No Voting", is_nda=True, users_can_vote=False) + question = Question.objects.create( + event=event, question="A question with enough text.", is_accepted=True + ) + client = Client() + client.force_login(make_user("staff", is_employee=True)) + resp = client.post( + f"/q/{question.id}/upvote", headers={"x-requested-with": "XMLHttpRequest"} + ) + assert resp.status_code == 302 + assert question.votes.count() == 0 + + @pytest.mark.django_db def test_archive_event_get_not_allowed(): """The endpoint accepts POST only (state-changing action).""" diff --git a/moderator/moderate/tests/test_utils.py b/moderator/moderate/tests/test_utils.py index d0e7d06..97d7847 100644 --- a/moderator/moderate/tests/test_utils.py +++ b/moderator/moderate/tests/test_utils.py @@ -1,7 +1,10 @@ import pytest +from django.conf import settings from django.contrib.auth.models import User +from django.test import override_settings from moderator.moderate.utils import ( + is_employee_groups, is_legacy_username, normalize_username, suggest_username, @@ -94,3 +97,41 @@ def test_is_legacy_username_accepts_with_suffix(): def test_is_legacy_username_accepts_with_special_chars(): # ., @, +, -, _ are all allowed by UnicodeUsernameValidator. assert is_legacy_username("user.name+tag-1_2") is False + + +def test_is_employee_groups_accepts_staff_group(): + assert is_employee_groups(["team_moco"]) is True + + +def test_is_employee_groups_rejects_nda_only(): + assert is_employee_groups(["mozilliansorg_nda"]) is False + + +def test_is_employee_groups_rejects_contingent_worker_nda(): + assert is_employee_groups(["mozilliansorg_contingentworkernda"]) is False + + +def test_is_employee_groups_accepts_staff_who_is_also_nda(): + assert is_employee_groups(["mozilliansorg_nda", "team_mzla"]) is True + + +def test_is_employee_groups_ignores_unknown_groups(): + """A group outside the configured groups must not confer staff status.""" + assert is_employee_groups(["mozilliansorg_random_community_group"]) is False + + +def test_is_employee_groups_rejects_empty(): + assert is_employee_groups([]) is False + + +@override_settings(EMPLOYEE_LOGIN_GROUPS=["team_standards"]) +def test_is_employee_groups_matches_names_exactly(): + """A staff group whose name happens to contain "nda" is still staff.""" + assert is_employee_groups(["team_standards"]) is True + + +def test_employee_and_nda_groups_are_disjoint(): + assert set(settings.EMPLOYEE_LOGIN_GROUPS).isdisjoint(settings.NDA_LOGIN_GROUPS) + assert set(settings.ALLOWED_LOGIN_GROUPS) == set( + settings.EMPLOYEE_LOGIN_GROUPS + ) | set(settings.NDA_LOGIN_GROUPS) diff --git a/moderator/moderate/utils.py b/moderator/moderate/utils.py index 5e4e48f..6f57306 100644 --- a/moderator/moderate/utils.py +++ b/moderator/moderate/utils.py @@ -1,6 +1,7 @@ import bisect from re import compile, escape +from django.conf import settings from django.contrib.auth.models import User from django.contrib.auth.validators import UnicodeUsernameValidator from django.core.exceptions import ValidationError @@ -37,7 +38,7 @@ def suggest_username(email): if existing_usernames: ids = [] for existing in existing_usernames: - i = existing[len(username):] + i = existing[len(username) :] if i: i = int(i) bisect.insort(ids, i) @@ -55,6 +56,15 @@ def suggest_username(email): return username +def is_employee_groups(groups): + """True if `groups` contains one of the staff groups. + + Employees reach the site through their LDAP derived groups, while NDA + community members only ever carry a group from NDA_LOGIN_GROUPS. + """ + return not set(groups).isdisjoint(settings.EMPLOYEE_LOGIN_GROUPS) + + def is_legacy_username(username): """True if `username` contains characters Django's UnicodeUsernameValidator rejects. diff --git a/moderator/moderate/views.py b/moderator/moderate/views.py index b81d300..82c487a 100644 --- a/moderator/moderate/views.py +++ b/moderator/moderate/views.py @@ -4,7 +4,6 @@ from django.contrib.auth.decorators import login_required from django.contrib.auth.mixins import LoginRequiredMixin from django.contrib.auth.models import User -from django.core.exceptions import ValidationError from django.core.mail import send_mail from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator from django.db.models import Count, Q @@ -19,7 +18,7 @@ from moderator.moderate.forms import EventForm, QuestionForm from moderator.moderate.models import Event, Question, Vote -from moderator.moderate.templatetags.helpers import can_moderate_event +from moderator.moderate.templatetags.helpers import can_access_event, can_moderate_event SUBJECT = "Question moderation update" @@ -42,6 +41,7 @@ def main(request): if user.is_authenticated: events = ( Event.objects.filter(archived=False) + .visible_to(user) .prefetch_related("moderators") .annotate( approved_count=Count( @@ -54,9 +54,6 @@ def main(request): ) .order_by("-event_date") ) - - if not user.userprofile.is_nda_member: - events = events.exclude(is_nda=True) return render(request, "index.jinja", {"events": events, "user": user}) return render(request, "index.jinja", {"user": user}) @@ -64,14 +61,9 @@ def main(request): @login_required(login_url="/") def archive(request): """List of all archived events.""" - q_args = { - "archived": True, - } - # Filter out NDA events for non-NDA users - if not request.user.userprofile.is_nda_member and not request.user.is_superuser: - q_args["is_nda"] = False events_list = ( - Event.objects.filter(**q_args) + Event.objects.filter(archived=True) + .visible_to(request.user) .annotate( approved_count=Count("questions", filter=Q(questions__is_accepted=True)) ) @@ -146,10 +138,7 @@ def moderate_event(request, slug, q_id=None, accepted=None): # Update the question if it's accepted or rejected if q_id: - try: - question = Question.objects.get(id=q_id) - except Question.DoesNotExist: - raise ValidationError("This question is not valid") + question = get_object_or_404(Question, id=q_id, event=event) question.is_accepted = accepted question.save() @@ -224,12 +213,11 @@ def show_event(request, e_slug, q_id=None): question = None user = request.user - # Do not display NDA events to non NDA members or non employees. - if event.is_nda and not user.userprofile.is_nda_member: + if not can_access_event(event, user): raise Http404 if q_id: - question = Question.objects.get(id=q_id) + question = get_object_or_404(Question, id=q_id, event=event) questions_q = Question.objects.filter(event=event, is_accepted=True).annotate( vote_count=Count("votes") @@ -286,14 +274,15 @@ def show_event(request, e_slug, q_id=None): def upvote(request, q_id): """Upvote question""" - question = Question.objects.get(pk=q_id) + question = get_object_or_404(Question, pk=q_id) event = question.event user = request.user - if not ( - user_can_vote := ( - event.users_can_vote or (event.is_nda and user.userprofile.is_nda_member) - ) - ): + + if not can_access_event(event, user): + raise Http404 + + user_can_vote = event.users_can_vote + if not user_can_vote: msg = "Voting is not allowed for this event." messages.warning(request, msg) diff --git a/moderator/settings.py b/moderator/settings.py index cefab8d..965fb1f 100644 --- a/moderator/settings.py +++ b/moderator/settings.py @@ -248,8 +248,8 @@ def _username_algo(email): OIDC_RP_SCOPES = "openid email profile" OIDC_USE_NONCE = config("OIDC_USE_NONCE", default=True, cast=bool) -# Allowed groups a user must have to login -ALLOWED_LOGIN_GROUPS = [ +# LDAP derived groups that identify staff +EMPLOYEE_LOGIN_GROUPS = [ "team_moco", "team_mofo", "team_mozillaonline", @@ -257,10 +257,17 @@ def _username_algo(email): "team_mzla", "team_mzvc", "team_mzai", +] + +# Groups that identify NDA community members +NDA_LOGIN_GROUPS = [ "mozilliansorg_nda", "mozilliansorg_contingentworkernda", ] +# Allowed groups a user must have to login +ALLOWED_LOGIN_GROUPS = EMPLOYEE_LOGIN_GROUPS + NDA_LOGIN_GROUPS + # Django 3.2 Autofield DEFAULT_AUTO_FIELD = "django.db.models.AutoField" diff --git a/setup.cfg b/setup.cfg index 2e82477..cb77bc1 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,3 +1,5 @@ [flake8] max-line-length=99 +# E203 contradicts black's slice formatting +extend-ignore=E203 exclude=migrations,moderator/settings/local.py