From 0da7e2e794f5714321f18212aafd86978fb00896 Mon Sep 17 00:00:00 2001 From: Youngseok Oh <247423326+YS-OH-CORE@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:46:04 +0900 Subject: [PATCH] Preserve verified plus addresses in dashboard signing links --- clabot/tests/test_views.py | 208 +++++++++++++++++++++++++++++++++++++ clabot/views.py | 15 ++- templates/dashboard.html | 6 +- 3 files changed, 223 insertions(+), 6 deletions(-) diff --git a/clabot/tests/test_views.py b/clabot/tests/test_views.py index 6f810a0..0358120 100644 --- a/clabot/tests/test_views.py +++ b/clabot/tests/test_views.py @@ -1,6 +1,13 @@ +import re +from html import unescape +from unittest.mock import AsyncMock, patch +from urllib.parse import parse_qs, urlencode, urlsplit + from django.test import TestCase from django_github_app.models import Installation, Repository +from cla.models import Agreement, PendingSignature, Signature + class TestHomePageView(TestCase): @classmethod @@ -39,3 +46,204 @@ def test_renders_list_of_repos(self): "
  • psf/requests
  • psf/black
    • ", html=True, ) + + +class TestDashboardView(TestCase): + @classmethod + def setUpTestData(cls): + cls.agreement = Agreement.objects.create( + title="Test Agreement", + description="An agreement for testing", + document="I agree!", + ) + installation = Installation.objects.create(installation_id=12345) + cls.repository = Repository.objects.create( + installation=installation, + repository_id=34567, + repository_node_id="test-repository", + full_name="example/test-repository", + ) + + def set_emails(self, emails): + session = self.client.session + session.update( + { + "github_login": "username", + "github_id": 123, + "github_node_id": "test-user", + "emails": emails, + } + ) + session.save() + + def create_pending(self, email, pull_number=1): + return PendingSignature.objects.create( + agreement=self.agreement, + github_repository_id=self.repository.repository_id, + pull_number=pull_number, + email_address=email, + ) + + def review_links(self, response): + return [ + unescape(url) + for url in re.findall(r'href="(/sign/?\?[^\"]+)"', response.content.decode()) + ] + + def assert_review_email(self, url, email): + self.assertEqual( + parse_qs(urlsplit(url).query), + {"agreement_id": [str(self.agreement.id)], "email_address": [email]}, + ) + + def test_verified_plus_email_can_sign(self): + email = "User+GitHub@example.com" + self.create_pending(email) + self.set_emails([{"email": email, "verified": True}]) + + response = self.client.get("/dashboard/") + self.assertContains(response, "Review and Sign", count=1) + self.assertNotContains(response, "Cannot proceed") + [url] = self.review_links(response) + self.assert_review_email(url, email) + + response = self.client.get(url, follow=True) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.context["email_address"], email) + self.assertFalse(Signature.objects.exists()) + self.assertEqual(PendingSignature.objects.count(), 1) + + # Exercise signing in the test database without sending anything to GitHub. + github = AsyncMock() + github.getitem.return_value = {"number": 1} + with ( + patch("clabot.views.AsyncGitHubAPI") as github_api, + patch("clabot.views.handle_pull_request", new_callable=AsyncMock) as handle_pull, + ): + github_api.return_value.__aenter__.return_value = github + response = self.client.post( + response.request["PATH_INFO"] + "?" + response.request["QUERY_STRING"] + ) + + self.assertRedirects(response, "/dashboard/", fetch_redirect_response=False) + signature = Signature.objects.get() + self.assertEqual(signature.email_address, email) + self.assertEqual(signature.signing_email_address, email) + self.assertEqual(signature.github_id, 123) + self.assertFalse(PendingSignature.objects.exists()) + github.getitem.assert_awaited_once_with("/repos/example/test-repository/pulls/1") + handle_pull.assert_awaited_once() + + def test_pending_aliases_share_one_review_link_to_a_verified_address(self): + self.create_pending("user+first-commit@example.com") + self.create_pending("user+second-commit@example.com", pull_number=2) + verified = ["User+GitHub@example.com", "user+other-verified@example.com"] + self.set_emails( + [{"email": "user@example.com", "verified": False}] + + [{"email": email, "verified": True} for email in verified] + ) + + response = self.client.get("/dashboard/") + self.assertContains(response, "Review and Sign", count=1) + self.assertNotContains(response, "Cannot proceed") + [url] = self.review_links(response) + email = parse_qs(urlsplit(url).query)["email_address"][0] + self.assertIn(email, verified) + self.assert_review_email(url, email) + response = self.client.get(url, follow=True) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.context["email_address"], email) + + def test_verified_base_address_is_preferred_to_alias(self): + self.create_pending("user+commit@example.com") + self.set_emails( + [ + {"email": "user+github@example.com", "verified": True}, + {"email": "user@example.com", "verified": True}, + ] + ) + + response = self.client.get("/dashboard/") + [url] = self.review_links(response) + self.assert_review_email(url, "user@example.com") + + def assert_signing_rejected(self, email): + url = "/sign/?" + urlencode({"agreement_id": self.agreement.id, "email_address": email}) + with ( + patch("clabot.views.AsyncGitHubAPI") as github_api, + patch("clabot.views.handle_pull_request", new_callable=AsyncMock) as handle_pull, + ): + for method in (self.client.get, self.client.post): + with self.subTest(method=method.__name__, email=email): + response = method(url) + self.assertRedirects(response, "/", fetch_redirect_response=False) + self.assertFalse(Signature.objects.exists()) + self.assertEqual(PendingSignature.objects.count(), 1) + github_api.assert_not_called() + handle_pull.assert_not_called() + + def test_unverified_plus_email_cannot_sign(self): + email = "user+github@example.com" + self.create_pending(email) + self.set_emails( + [ + {"email": email, "verified": False}, + {"email": "someone-else@example.com", "verified": True}, + ] + ) + + response = self.client.get("/dashboard/") + self.assertContains(response, "Cannot proceed") + self.assertEqual(self.review_links(response), []) + self.assert_signing_rejected(email) + + def test_signing_still_requires_the_exact_verified_address(self): + self.create_pending("user+commit@example.com") + self.set_emails( + [ + {"email": "user+github@example.com", "verified": True}, + {"email": "user+unverified@example.com", "verified": False}, + ] + ) + + for email in ("user@example.com", "user+unverified@example.com", "user+other@example.com"): + self.assert_signing_rejected(email) + + def test_plain_verified_email_can_review_agreement(self): + email = "user@example.com" + self.create_pending(email) + self.set_emails([{"email": email, "verified": True}]) + + response = self.client.get("/dashboard/") + [url] = self.review_links(response) + self.assert_review_email(url, email) + response = self.client.get(url, follow=True) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.context["email_address"], email) + self.assertIsNone(response.context["form"]) + + def test_noreply_verified_email_can_review_agreement(self): + for email in ( + "12345+username@users.noreply.github.com", + "username@users.noreply.github.com", + ): + with self.subTest(email=email): + self.create_pending(email) + self.set_emails( + [ + {"email": email, "verified": True}, + {"email": "contact+cla@example.com", "verified": True}, + {"email": "unverified@example.com", "verified": False}, + ] + ) + + response = self.client.get("/dashboard/") + [url] = self.review_links(response) + self.assert_review_email(url, email) + response = self.client.get(url, follow=True) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.context["email_address"], email) + self.assertEqual( + response.context["form"].fields["email"].choices, + [("", "---"), ("contact+cla@example.com", "contact+cla@example.com")], + ) diff --git a/clabot/views.py b/clabot/views.py index 08bf2b9..3060552 100644 --- a/clabot/views.py +++ b/clabot/views.py @@ -42,9 +42,18 @@ def get_context_data(self, **kwargs): normalize_email(e["email"]) for e in self.request.session["emails"] ] ) - context["verified_emails"] = [ - e["email"].lower() for e in self.request.session["emails"] if e["verified"] - ] + verified_emails = {} + for email in self.request.session["emails"]: + if email["verified"]: + address = email["email"] + normalized = normalize_email(address) + # Prefer the untagged address when it is also verified. + if normalized not in verified_emails or address.lower() == normalized: + verified_emails[normalized] = address + for pending_signature in context["pending_signatures"]: + pending_signature.verified_email = verified_emails.get( + pending_signature.normalized_email + ) context["existing_signatures"] = Signature.objects.filter( Q(github_login=self.request.session["github_login"]) | Q(github_id=self.request.session["github_id"]) diff --git a/templates/dashboard.html b/templates/dashboard.html index 35bcfac..31c10c2 100644 --- a/templates/dashboard.html +++ b/templates/dashboard.html @@ -18,9 +18,9 @@

    {{ agreement.grouper }}

    For {{ pending_for_agreement_by_ref|length }} commits across {{ pending_for_agreement_by_repo|length }} repositories - with email address {{ email.grouper }}. + with email address {{ email.list.0.verified_email|default:email.grouper }}.

    - {% if email.grouper|lower not in verified_emails %} + {% if not email.list.0.verified_email %}

    Cannot proceed, {{ email.grouper }} @@ -31,7 +31,7 @@

    {{ agreement.grouper }}

    {% else %}
    {% endif %}