From a8ca0879e6414e4c7a6bde17f15d7781077b5fbd Mon Sep 17 00:00:00 2001 From: connorhaggerty Date: Thu, 23 Jul 2026 18:51:46 -0400 Subject: [PATCH 1/4] Read CSRF_TRUSTED_ORIGINS from the environment (#2) Django rejected logins with 'Origin checking failed' when Jackil was reached through a reverse proxy or an external hostname because no trusted origins were configured. Add an env-driven CSRF_TRUSTED_ORIGINS setting and document it for proxy deployments alongside ALLOWED_HOSTS. --- README.md | 13 +++++++++++++ config/settings.py | 8 ++++++++ 2 files changed, 21 insertions(+) diff --git a/README.md b/README.md index 80cddef..5517af6 100644 --- a/README.md +++ b/README.md @@ -80,6 +80,19 @@ Visit `http://localhost:8000`. Demo logins: `admin` / `admin12345` (admin), docker compose up --build ``` +### Behind a reverse proxy / external hostname + +When Jackil is reached through a proxy or a hostname other than `localhost`, +set both env vars so Django accepts logins and other POSTs: + +```bash +ALLOWED_HOSTS=jackil-tail.kingdom.local,help.acme.com +CSRF_TRUSTED_ORIGINS=https://jackil-tail.kingdom.local,https://help.acme.com +``` + +`CSRF_TRUSTED_ORIGINS` must include the scheme. Without it, POSTs fail with +`Forbidden (Origin checking failed … does not match any trusted origins.)`. + ## Background jobs (cron) ```bash diff --git a/config/settings.py b/config/settings.py index 3ddf617..7e44f79 100644 --- a/config/settings.py +++ b/config/settings.py @@ -9,6 +9,14 @@ DEBUG = config("DEBUG", default=True, cast=bool) ALLOWED_HOSTS = config("ALLOWED_HOSTS", default="localhost,127.0.0.1").split(",") +# Origins trusted for unsafe (POST) requests. Required when the app is reached +# through a reverse proxy or an external hostname, otherwise Django rejects the +# login/POST with "Origin checking failed". Comma-separated, scheme included, +# e.g. CSRF_TRUSTED_ORIGINS=https://jackil-tail.kingdom.local,https://help.acme.com +CSRF_TRUSTED_ORIGINS = [ + o.strip() for o in config("CSRF_TRUSTED_ORIGINS", default="").split(",") if o.strip() +] + # Database — Postgres in production, SQLite for zero-dependency local dev. # Set USE_SQLITE=1 in your .env to develop without a Postgres server. USE_SQLITE = config("USE_SQLITE", default=False, cast=bool) From a2366530b5e8cef82541299cac46823f830bcad5 Mon Sep 17 00:00:00 2001 From: connorhaggerty Date: Thu, 23 Jul 2026 18:51:46 -0400 Subject: [PATCH 2/4] Pause the SLA clock on Pending and recompute on priority change (#3) The time-sensitivity timer kept running while a ticket sat in Pending and never recalculated when the priority changed, so escalations used stale deadlines. Freeze the clock while a ticket is in a paused status and shift the due dates forward by the paused span when it resumes, banking the time so later recalculations stay correct. Recompute first_response_due and resolution_due against the new target whenever the priority changes. Stop check_sla from breaching Pending tickets. --- apps/sla/management/commands/check_sla.py | 3 +- apps/sla/receivers.py | 27 ++++- apps/sla/service.py | 68 ++++++++++- apps/sla/tests.py | 111 ++++++++++++++++++ apps/tickets/events.py | 4 + ...sla_paused_at_ticket_sla_paused_seconds.py | 23 ++++ apps/tickets/models.py | 5 + apps/tickets/views.py | 14 ++- 8 files changed, 247 insertions(+), 8 deletions(-) create mode 100644 apps/tickets/migrations/0009_ticket_sla_paused_at_ticket_sla_paused_seconds.py diff --git a/apps/sla/management/commands/check_sla.py b/apps/sla/management/commands/check_sla.py index 79b5fab..4b6e76c 100644 --- a/apps/sla/management/commands/check_sla.py +++ b/apps/sla/management/commands/check_sla.py @@ -11,7 +11,8 @@ def add_arguments(self, parser): parser.add_argument("--dry-run", action="store_true") def handle(self, *args, **options): - qs = Ticket.objects.filter(status__in=["open", "in_progress", "pending"]) + # Pending tickets have a paused SLA clock and must not breach. + qs = Ticket.objects.filter(status__in=["open", "in_progress"]) breached = 0 for ticket in qs: newly = check_and_flag_breaches(ticket) diff --git a/apps/sla/receivers.py b/apps/sla/receivers.py index 23729cd..5c83800 100644 --- a/apps/sla/receivers.py +++ b/apps/sla/receivers.py @@ -1,6 +1,11 @@ from django.dispatch import receiver -from apps.tickets.events import ticket_created, ticket_replied +from apps.tickets.events import ( + ticket_created, + ticket_priority_changed, + ticket_replied, + ticket_status_changed, +) @receiver(ticket_created) @@ -10,6 +15,26 @@ def on_ticket_created(sender, ticket, **kwargs): apply_sla(ticket) +@receiver(ticket_status_changed) +def on_ticket_status_changed(sender, ticket, actor, new_status, **kwargs): + """Pause the SLA clock while the ticket sits in a paused status (Pending), + and resume it — shifting due dates by the paused span — when it leaves.""" + from .service import PAUSED_STATUSES, pause_sla, resume_sla + + if new_status in PAUSED_STATUSES: + pause_sla(ticket) + else: + resume_sla(ticket) + + +@receiver(ticket_priority_changed) +def on_ticket_priority_changed(sender, ticket, actor, old_priority, new_priority, **kwargs): + """Recompute the due dates against the new priority's SLA target.""" + from .service import apply_sla + + apply_sla(ticket) + + @receiver(ticket_replied) def on_ticket_replied(sender, ticket, author, kind, **kwargs): if ( diff --git a/apps/sla/service.py b/apps/sla/service.py index f04da3e..609d4e3 100644 --- a/apps/sla/service.py +++ b/apps/sla/service.py @@ -54,25 +54,83 @@ def add_working_minutes(start, minutes, schedule): return cursor +# Ticket statuses during which the SLA clock is frozen. +PAUSED_STATUSES = {"pending"} + + def applicable_target(ticket): return SLATarget.objects.filter(active=True, priority=ticket.priority).first() def apply_sla(ticket, save=True): """Compute and set first_response_due / resolution_due from the ticket's - priority target. No-op if there is no matching target.""" + priority target. No-op if there is no matching target. + + Any time already banked in ``sla_paused_seconds`` is added back so that a + priority change recomputes the targets without discarding earned pause + credit. Called on ticket creation and on every priority change.""" target = applicable_target(ticket) if target is None: return False schedule = BusinessSchedule.active() base = ticket.created_at or timezone.now() - ticket.first_response_due = add_working_minutes(base, target.response_minutes, schedule) - ticket.resolution_due = add_working_minutes(base, target.resolution_minutes, schedule) + paused = timedelta(seconds=ticket.sla_paused_seconds or 0) + ticket.first_response_due = ( + add_working_minutes(base, target.response_minutes, schedule) + paused + ) + ticket.resolution_due = add_working_minutes(base, target.resolution_minutes, schedule) + paused if save: ticket.save(update_fields=["first_response_due", "resolution_due"]) return True +def _effective_now(ticket, now): + """The clock reading to judge an open target against. While the ticket is + paused it is frozen at the instant the pause began, so a ticket sitting in + Pending never drifts into 'overdue'.""" + if ticket.sla_paused_at is not None: + return min(now, ticket.sla_paused_at) + return now + + +def pause_sla(ticket, when=None): + """Freeze the SLA clock (called when a ticket enters a paused status). + Idempotent and a no-op when the ticket has no SLA targets.""" + if ticket.sla_paused_at is not None: + return False + if ticket.first_response_due is None and ticket.resolution_due is None: + return False + ticket.sla_paused_at = when or timezone.now() + ticket.save(update_fields=["sla_paused_at"]) + return True + + +def resume_sla(ticket, when=None): + """Resume a paused clock: push the still-open due dates forward by however + long the ticket was paused, and bank that time in sla_paused_seconds.""" + if ticket.sla_paused_at is None: + return False + now = when or timezone.now() + delta = now - ticket.sla_paused_at + if delta.total_seconds() < 0: + delta = timedelta(0) + if ticket.first_response_due is not None and ticket.first_responded_at is None: + ticket.first_response_due += delta + if ticket.resolution_due is not None and ticket.status not in ("resolved", "closed"): + ticket.resolution_due += delta + ticket.sla_paused_seconds = (ticket.sla_paused_seconds or 0) + int(delta.total_seconds()) + ticket.sla_paused_at = None + ticket.save( + update_fields=[ + "first_response_due", + "resolution_due", + "sla_paused_seconds", + "sla_paused_at", + ] + ) + return True + + def mark_first_response(ticket, when=None): """Record the first agent response time (idempotent).""" if ticket.first_responded_at is None: @@ -88,7 +146,7 @@ def response_status(ticket, now=None): return "none" if ticket.first_responded_at is not None: return "met" if ticket.first_responded_at <= ticket.first_response_due else "breached" - now = now or timezone.now() + now = _effective_now(ticket, now or timezone.now()) return "overdue" if now > ticket.first_response_due else "open" @@ -97,7 +155,7 @@ def resolution_status(ticket, now=None): return "none" if ticket.status in ("resolved", "closed") and ticket.closed_at is not None: return "met" if ticket.closed_at <= ticket.resolution_due else "breached" - now = now or timezone.now() + now = _effective_now(ticket, now or timezone.now()) return "overdue" if now > ticket.resolution_due else "open" diff --git a/apps/sla/tests.py b/apps/sla/tests.py index 0c09a8f..4c63b3a 100644 --- a/apps/sla/tests.py +++ b/apps/sla/tests.py @@ -246,6 +246,117 @@ def test_first_agent_reply_marks_response(self): # first_responded_at should remain the original (idempotent) +class PauseResumeTests(TestCase): + def _paused_ticket(self, **kwargs): + now = timezone.now() + defaults = dict( + title="Pause test", + priority="high", + status="open", + created_by=_make_user(), + first_response_due=now + timedelta(hours=1), + resolution_due=now + timedelta(hours=4), + first_responded_at=None, + ) + defaults.update(kwargs) + return Ticket.objects.create(**defaults) + + def test_pause_freezes_clock_no_false_overdue(self): + from apps.sla.service import pause_sla, response_status + + ticket = self._paused_ticket() + paused_at = ticket.first_response_due - timedelta(minutes=10) + pause_sla(ticket, when=paused_at) + self.assertIsNotNone(ticket.sla_paused_at) + # An hour past the due date, but the frozen clock keeps it "open". + later = ticket.first_response_due + timedelta(hours=1) + self.assertEqual(response_status(ticket, now=later), "open") + + def test_resume_shifts_due_dates_and_banks_time(self): + from apps.sla.service import pause_sla, resume_sla + + ticket = self._paused_ticket() + original_response_due = ticket.first_response_due + original_resolution_due = ticket.resolution_due + paused_at = timezone.now() + pause_sla(ticket, when=paused_at) + resume_at = paused_at + timedelta(hours=2) + resume_sla(ticket, when=resume_at) + self.assertIsNone(ticket.sla_paused_at) + self.assertEqual(ticket.sla_paused_seconds, 2 * 3600) + self.assertEqual(ticket.first_response_due, original_response_due + timedelta(hours=2)) + self.assertEqual(ticket.resolution_due, original_resolution_due + timedelta(hours=2)) + + def test_resume_without_pause_is_noop(self): + from apps.sla.service import resume_sla + + ticket = self._paused_ticket() + self.assertFalse(resume_sla(ticket)) + + def test_status_change_signal_pauses_and_resumes(self): + """open → pending pauses; pending → open resumes and banks the span.""" + from apps.tickets.events import ticket_status_changed + + ticket = self._paused_ticket() + original_resolution_due = ticket.resolution_due + ticket_status_changed.send(sender=Ticket, ticket=ticket, actor=None, new_status="pending") + ticket.refresh_from_db() + self.assertIsNotNone(ticket.sla_paused_at) + ticket_status_changed.send(sender=Ticket, ticket=ticket, actor=None, new_status="open") + ticket.refresh_from_db() + # The pause was released (span is ~0s in-test; real banking is covered + # by test_resume_shifts_due_dates_and_banks_time). + self.assertIsNone(ticket.sla_paused_at) + self.assertGreaterEqual(ticket.sla_paused_seconds, 0) + self.assertGreaterEqual(ticket.resolution_due, original_resolution_due) + + +class PriorityRecalcTests(TestCase): + def test_priority_change_recomputes_due_dates(self): + from apps.sla.service import apply_sla + + SLATarget.objects.update_or_create( + priority="low", + defaults={"response_minutes": 480, "resolution_minutes": 2880}, + ) + SLATarget.objects.update_or_create( + priority="critical", + defaults={"response_minutes": 30, "resolution_minutes": 120}, + ) + created = timezone.make_aware(datetime(2025, 1, 6, 10, 0)) + ticket = Ticket.objects.create( + title="Recalc test", priority="low", status="open", created_by=_make_user() + ) + Ticket.objects.filter(pk=ticket.pk).update(created_at=created) + ticket.refresh_from_db() + apply_sla(ticket) + low_due = ticket.first_response_due + # Escalate to critical → tighter target → sooner due date. + ticket.priority = "critical" + apply_sla(ticket) + self.assertLess(ticket.first_response_due, low_due) + self.assertEqual(ticket.first_response_due, created + timedelta(minutes=30)) + + def test_priority_recalc_keeps_banked_pause(self): + from apps.sla.service import apply_sla + + SLATarget.objects.update_or_create( + priority="high", + defaults={"response_minutes": 120, "resolution_minutes": 480}, + ) + created = timezone.make_aware(datetime(2025, 1, 6, 10, 0)) + ticket = Ticket.objects.create( + title="Bank test", priority="high", status="open", created_by=_make_user() + ) + Ticket.objects.filter(pk=ticket.pk).update(created_at=created, sla_paused_seconds=3600) + ticket.refresh_from_db() + apply_sla(ticket) + # 120-min target + 1h banked pause = 3h after creation. + self.assertEqual( + ticket.first_response_due, created + timedelta(minutes=120) + timedelta(hours=1) + ) + + class EscalationTests(TestCase): def test_escalate_bumps_priority_on_resolution_breach(self): from apps.sla.service import escalate diff --git a/apps/tickets/events.py b/apps/tickets/events.py index d3e58e2..32fcecf 100644 --- a/apps/tickets/events.py +++ b/apps/tickets/events.py @@ -10,3 +10,7 @@ # Emitted after a ticket's status changes. # kwargs: ticket, actor, new_status ticket_status_changed = Signal() + +# Emitted after a ticket's priority changes. +# kwargs: ticket, actor, old_priority, new_priority +ticket_priority_changed = Signal() diff --git a/apps/tickets/migrations/0009_ticket_sla_paused_at_ticket_sla_paused_seconds.py b/apps/tickets/migrations/0009_ticket_sla_paused_at_ticket_sla_paused_seconds.py new file mode 100644 index 0000000..72010d7 --- /dev/null +++ b/apps/tickets/migrations/0009_ticket_sla_paused_at_ticket_sla_paused_seconds.py @@ -0,0 +1,23 @@ +# Generated by Django 5.2.16 on 2026-07-23 22:48 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('tickets', '0008_delete_ticketcomment'), + ] + + operations = [ + migrations.AddField( + model_name='ticket', + name='sla_paused_at', + field=models.DateTimeField(blank=True, null=True), + ), + migrations.AddField( + model_name='ticket', + name='sla_paused_seconds', + field=models.PositiveIntegerField(default=0), + ), + ] diff --git a/apps/tickets/models.py b/apps/tickets/models.py index 2a5e33e..ccd47c1 100644 --- a/apps/tickets/models.py +++ b/apps/tickets/models.py @@ -51,6 +51,11 @@ class Ticket(models.Model): first_responded_at = models.DateTimeField(null=True, blank=True) response_breached = models.BooleanField(default=False) resolution_breached = models.BooleanField(default=False) + # SLA clock pausing (e.g. while Pending). sla_paused_at is set while the + # clock is frozen; sla_paused_seconds accumulates across pause cycles so + # due dates can be recomputed correctly after a priority change. + sla_paused_at = models.DateTimeField(null=True, blank=True) + sla_paused_seconds = models.PositiveIntegerField(default=0) class Meta: ordering = ["-created_at"] diff --git a/apps/tickets/views.py b/apps/tickets/views.py index cc9a1be..f4def62 100644 --- a/apps/tickets/views.py +++ b/apps/tickets/views.py @@ -270,7 +270,9 @@ def _handle_manage(request, ticket): ticket.due_at = new_due new_priority = request.POST.get("priority", ticket.priority) - if new_priority != ticket.priority: + priority_changed = new_priority != ticket.priority + old_priority = ticket.priority + if priority_changed: record_system_event( ticket, actor, @@ -295,6 +297,16 @@ def _handle_manage(request, ticket): ticket.closed_at = None ticket.save() + if priority_changed: + from .events import ticket_priority_changed + + ticket_priority_changed.send( + sender=Ticket, + ticket=ticket, + actor=actor, + old_priority=old_priority, + new_priority=new_priority, + ) if status_changed: from .events import ticket_status_changed From 104c9615e9eb0b4f49ba17b682aff0ef22a26803 Mon Sep 17 00:00:00 2001 From: connorhaggerty Date: Thu, 23 Jul 2026 18:51:46 -0400 Subject: [PATCH 3/4] Add issue templates and community health files Bring the bug report, feature request, config, pull request template, contributing guide, and security policy in line with the other SQSY repos, adapted to Jackil's helpdesk domain. --- .github/CONTRIBUTING.md | 42 +++++++++++++ .github/ISSUE_TEMPLATE/bug_report.yml | 71 ++++++++++++++++++++++ .github/ISSUE_TEMPLATE/config.yml | 5 ++ .github/ISSUE_TEMPLATE/feature_request.yml | 43 +++++++++++++ .github/SECURITY.md | 35 +++++++++++ .github/pull_request_template.md | 38 ++++++++++++ 6 files changed, 234 insertions(+) create mode 100644 .github/CONTRIBUTING.md create mode 100644 .github/ISSUE_TEMPLATE/bug_report.yml create mode 100644 .github/ISSUE_TEMPLATE/config.yml create mode 100644 .github/ISSUE_TEMPLATE/feature_request.yml create mode 100644 .github/SECURITY.md create mode 100644 .github/pull_request_template.md diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md new file mode 100644 index 0000000..d5f35a7 --- /dev/null +++ b/.github/CONTRIBUTING.md @@ -0,0 +1,42 @@ +# Contributing standards + +Conventions for issues, branches, commits, and pull requests in Jackil. The +templates in `.github/` enforce most of this — this is the why. + +## Issues + +- Use the Bug report or Feature request form (blank issues are disabled). +- One issue = one problem or one request. +- Security issues go through a private advisory, never a public issue. +- Never paste secrets, SMTP/IMAP credentials, or customer data. + +## Branches + +Short, prefixed, kebab-case off `main`: + +``` +fix/sla-pause-on-pending +feat/kb-article-versioning +docs/proxy-deployment +chore/bump-2026.2.1 +``` + +## Commits + +- Plain style. Short subject with the version in parens when it's a release, + e.g. `Pause the SLA clock on Pending and recompute on priority change (2026.2.1)`. +- One sentence per change on its own line in the body; no bullet lists. +- **No AI / Co-Authored-By trailers.** +- Don't commit secrets, keys, or customer data. + +## Pull requests + +- Fill in the PR template, including the proposed commit message. +- Tests must pass: `USE_SQLITE=1 .venv/bin/python manage.py test`. +- Run `makemigrations` if a model changed, and commit the migration. +- Keep lint clean: `.venv/bin/ruff check .` and `.venv/bin/ruff format --check .`. + +## Releases + +Jackil's version lives in git tags (there is no in-code version string). A +release is a `vYYYY.N.P` tag on `main` after the PR merges. diff --git a/.github/ISSUE_TEMPLATE/bug_report.yml b/.github/ISSUE_TEMPLATE/bug_report.yml new file mode 100644 index 0000000..2dc3440 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/bug_report.yml @@ -0,0 +1,71 @@ +name: Bug report +description: Something in Jackil isn't working as expected +labels: ["bug", "needs-triage"] +body: + - type: markdown + attributes: + value: | + Thanks for filing a bug. One issue = one problem. Never paste secrets, + API keys, SMTP/IMAP passwords, or customer data — redact before posting. + - type: input + id: version + attributes: + label: Jackil version + description: Release tag (e.g. v2026.2.1) or the deployed commit SHA. + placeholder: "v2026.2.1" + validations: + required: true + - type: dropdown + id: component + attributes: + label: Area + options: + - Tickets + - SLA / escalation / time sensitivity + - Inbox / inbound & outbound email + - Knowledge base / Help Center + - Automation / macros + - Custom fields / request forms + - Reports & analytics + - REST API / webhooks + - Notifications + - Admin console / settings / branding + - Civil SSO + - Deployment (Docker / proxy / CSRF) + - Other + validations: + required: true + - type: dropdown + id: deployment + attributes: + label: Deployment + options: + - Docker Compose + - Manual / from source + - Other + - type: textarea + id: what-happened + attributes: + label: What happened + description: What you did, what you expected, and what actually happened. + validations: + required: true + - type: textarea + id: repro + attributes: + label: Steps to reproduce + placeholder: | + 1. ... + 2. ... + 3. ... + - type: textarea + id: logs + attributes: + label: Relevant logs / traceback + description: Django logs or the traceback. Redact secrets and customer data. + render: shell + - type: input + id: env + attributes: + label: Browser / OS (for UI bugs) + placeholder: "Firefox 141 · Ubuntu 24.04" diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml new file mode 100644 index 0000000..c3cb679 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/config.yml @@ -0,0 +1,5 @@ +blank_issues_enabled: false +contact_links: + - name: Report a security vulnerability + url: https://github.com/Susquehanna-Syntax/Jackil/security/advisories/new + about: Disclose security issues privately. Never open a public issue for a vulnerability. diff --git a/.github/ISSUE_TEMPLATE/feature_request.yml b/.github/ISSUE_TEMPLATE/feature_request.yml new file mode 100644 index 0000000..cbf7508 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/feature_request.yml @@ -0,0 +1,43 @@ +name: Feature request +description: Suggest an improvement to Jackil +labels: ["enhancement", "needs-triage"] +body: + - type: markdown + attributes: + value: | + One request per issue. Describe the problem before the solution — it + helps us find the best fix, which isn't always the one first imagined. + - type: textarea + id: problem + attributes: + label: Problem / use case + description: What are you trying to do, and what's missing or awkward today? + validations: + required: true + - type: textarea + id: proposal + attributes: + label: Proposed solution + - type: dropdown + id: area + attributes: + label: Area + options: + - Tickets + - SLA / escalation + - Inbox / email + - Knowledge base + - Automation / macros + - Custom fields / forms + - Reports & analytics + - REST API / webhooks + - Notifications + - Admin console / settings + - Civil SSO + - Other + validations: + required: true + - type: textarea + id: alternatives + attributes: + label: Alternatives considered diff --git a/.github/SECURITY.md b/.github/SECURITY.md new file mode 100644 index 0000000..8429ec2 --- /dev/null +++ b/.github/SECURITY.md @@ -0,0 +1,35 @@ +# Security Policy + +Jackil handles support tickets, inbound/outbound email, and customer data, so +we ask that security problems be disclosed privately. + +## Reporting a vulnerability + +**Do not open a public issue, PR, or discussion for a security problem.** + +Report it privately through GitHub: + +1. Go to the repository's **Security** tab → **Report a vulnerability** + (). +2. Include the details below. + +This opens a private advisory visible only to you and the maintainers. + +Please include: + +- Affected area (a specific view/endpoint, email ingestion, the API) and version. +- Steps to reproduce or a proof of concept. +- Impact — what an attacker can read, change, or do. +- Any suggested remediation. + +## Scope + +In scope: authentication and session handling, ticket/attachment access +control, email ingestion, the REST API and webhooks, CSRF/host handling, and +privilege boundaries between roles (customer / agent / admin). + +Out of scope: findings that require a pre-compromised host or admin account, +and issues in third-party dependencies without a Jackil-specific exploit path. + +We aim to acknowledge reports within a few days and to coordinate a fix and +disclosure timeline with you. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 0000000..0b8f26e --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,38 @@ +## Summary + + + +## Type + +- [ ] Bug fix +- [ ] Feature +- [ ] Refactor / cleanup +- [ ] Docs + +## Proposed commit message + + + +``` +``` + +## Checklist + +- [ ] Tests pass: `USE_SQLITE=1 .venv/bin/python manage.py test` +- [ ] Migrations added if any model changed (`makemigrations`) +- [ ] Lint clean: `.venv/bin/ruff check .` and `.venv/bin/ruff format --check .` +- [ ] Version bumped / tag planned if this is a release +- [ ] No AI / Co-Authored-By attribution in commits +- [ ] No secrets, keys, SMTP/IMAP passwords, or customer data committed + +## Testing + + + +## Screenshots + + From cf524c06600260c683c3022ecacff95298b97728 Mon Sep 17 00:00:00 2001 From: connorhaggerty Date: Thu, 23 Jul 2026 19:13:06 -0400 Subject: [PATCH 4/4] Apply ruff format and lint fixes to civilsso The Civil SSO app predated the ruff format/lint gate. Reformat it, add a __str__ to CachedCivilKey, order the model magic methods per DJ012, and drop an unused import so the format and lint checks pass. --- apps/civilsso/client.py | 10 +++-- apps/civilsso/models.py | 12 ++++-- apps/civilsso/tests.py | 89 +++++++++++++++++++++++------------------ apps/civilsso/views.py | 13 +++--- 4 files changed, 72 insertions(+), 52 deletions(-) diff --git a/apps/civilsso/client.py b/apps/civilsso/client.py index b09b863..aa03055 100644 --- a/apps/civilsso/client.py +++ b/apps/civilsso/client.py @@ -18,6 +18,7 @@ def _db_config(): from apps.civilsso.models import CivilConfig + try: return CivilConfig.current() except Exception: # noqa: BLE001 — pre-migration or DB down: env-only mode @@ -29,7 +30,7 @@ def civil_url() -> str: if env: return env cfg = _db_config() - return (cfg.url.rstrip("/") if cfg and cfg.enabled and cfg.url else "") + return cfg.url.rstrip("/") if cfg and cfg.enabled and cfg.url else "" def app_slug() -> str: @@ -75,8 +76,11 @@ def verify_sso_token(token: str) -> dict | None: return None try: return jwt.decode( - token, pem, algorithms=["EdDSA"], - audience=app_slug(), issuer="civil", + token, + pem, + algorithms=["EdDSA"], + audience=app_slug(), + issuer="civil", ) except jwt.PyJWTError as exc: logger.warning("Civil SSO token rejected: %s", exc) diff --git a/apps/civilsso/models.py b/apps/civilsso/models.py index 7baddd0..b4bb249 100644 --- a/apps/civilsso/models.py +++ b/apps/civilsso/models.py @@ -19,7 +19,8 @@ class CivilIdentity(models.Model): """ user = models.OneToOneField( - settings.AUTH_USER_MODEL, on_delete=models.CASCADE, + settings.AUTH_USER_MODEL, + on_delete=models.CASCADE, related_name="civil_identity", ) civil_id = models.UUIDField(unique=True, db_index=True) @@ -41,6 +42,9 @@ class CachedCivilKey(models.Model): fetched_from = models.URLField() fetched_at = models.DateTimeField(auto_now=True) + def __str__(self) -> str: + return f"civil key from {self.fetched_from or '(unset)'}" + @classmethod def current(cls) -> str: row = cls.objects.order_by("-fetched_at").first() @@ -60,10 +64,10 @@ class CivilConfig(models.Model): app_slug = models.SlugField(max_length=50, blank=True, default="") updated_at = models.DateTimeField(auto_now=True) + def __str__(self) -> str: + return f"civil:{self.url or '(unset)'} ({'on' if self.enabled else 'off'})" + @classmethod def current(cls) -> "CivilConfig": row = cls.objects.first() return row if row is not None else cls.objects.create() - - def __str__(self) -> str: - return f"civil:{self.url or '(unset)'} ({'on' if self.enabled else 'off'})" diff --git a/apps/civilsso/tests.py b/apps/civilsso/tests.py index 4a9ee43..14df4b0 100644 --- a/apps/civilsso/tests.py +++ b/apps/civilsso/tests.py @@ -14,21 +14,33 @@ KEY = Ed25519PrivateKey.generate() PRIVATE_PEM = KEY.private_bytes( - serialization.Encoding.PEM, serialization.PrivateFormat.PKCS8, - serialization.NoEncryption()).decode() -PUBLIC_PEM = KEY.public_key().public_bytes( - serialization.Encoding.PEM, serialization.PublicFormat.SubjectPublicKeyInfo, + serialization.Encoding.PEM, serialization.PrivateFormat.PKCS8, serialization.NoEncryption() ).decode() - - -def forge(sub=None, aud="jackil", iss="civil", exp_delta=60, username="alice", - key=PRIVATE_PEM, **extra): +PUBLIC_PEM = ( + KEY.public_key() + .public_bytes( + serialization.Encoding.PEM, + serialization.PublicFormat.SubjectPublicKeyInfo, + ) + .decode() +) + + +def forge( + sub=None, aud="jackil", iss="civil", exp_delta=60, username="alice", key=PRIVATE_PEM, **extra +): now = int(time.time()) claims = { - "iss": iss, "aud": aud, "sub": sub or str(uuid.uuid4()), - "preferred_username": username, "email": "a@example.com", - "name": "Alice Q Example", "orgs": [], - "iat": now, "exp": now + exp_delta, **extra, + "iss": iss, + "aud": aud, + "sub": sub or str(uuid.uuid4()), + "preferred_username": username, + "email": "a@example.com", + "name": "Alice Q Example", + "orgs": [], + "iat": now, + "exp": now + exp_delta, + **extra, } return jwt.encode(claims, key, algorithm="EdDSA") @@ -47,22 +59,21 @@ def _start_state(self): def test_valid_token_provisions_and_logs_in(self): self._start_state() sub = str(uuid.uuid4()) - resp = self.client.get("/accounts/civil/callback", - {"token": forge(sub=sub), "state": "st4te"}) + resp = self.client.get( + "/accounts/civil/callback", {"token": forge(sub=sub), "state": "st4te"} + ) self.assertEqual(resp.status_code, 302) self.assertEqual(resp["Location"], "/") identity = CivilIdentity.objects.get(civil_id=sub) self.assertEqual(identity.user.username, "alice") self.assertFalse(identity.user.has_usable_password()) - self.assertEqual(int(self.client.session["_auth_user_id"]), - identity.user.pk) + self.assertEqual(int(self.client.session["_auth_user_id"]), identity.user.pk) def test_second_login_reuses_mapping(self): sub = str(uuid.uuid4()) for _ in range(2): self._start_state() - self.client.get("/accounts/civil/callback", - {"token": forge(sub=sub), "state": "st4te"}) + self.client.get("/accounts/civil/callback", {"token": forge(sub=sub), "state": "st4te"}) self.assertEqual(CivilIdentity.objects.count(), 1) self.assertEqual(get_user_model().objects.count(), 1) @@ -70,16 +81,14 @@ def test_username_collision_gets_suffix_not_takeover(self): # A pre-existing local "alice" must NOT be claimable via Civil. local = get_user_model().objects.create_user("alice", password="x") self._start_state() - self.client.get("/accounts/civil/callback", - {"token": forge(), "state": "st4te"}) + self.client.get("/accounts/civil/callback", {"token": forge(), "state": "st4te"}) identity = CivilIdentity.objects.get() self.assertEqual(identity.user.username, "alice-2") self.assertNotEqual(identity.user.pk, local.pk) def test_state_mismatch_fails_to_login_page(self): self._start_state() - resp = self.client.get("/accounts/civil/callback", - {"token": forge(), "state": "WRONG"}) + resp = self.client.get("/accounts/civil/callback", {"token": forge(), "state": "WRONG"}) self.assertEqual(resp.status_code, 302) self.assertIn("/login/", resp["Location"]) self.assertEqual(CivilIdentity.objects.count(), 0) @@ -87,8 +96,10 @@ def test_state_mismatch_fails_to_login_page(self): def test_bad_tokens_fail_closed(self): stranger = Ed25519PrivateKey.generate() stranger_pem = stranger.private_bytes( - serialization.Encoding.PEM, serialization.PrivateFormat.PKCS8, - serialization.NoEncryption()).decode() + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + serialization.NoEncryption(), + ).decode() for label, token in [ ("wrong audience", forge(aud="vigil")), ("wrong issuer", forge(iss="evil")), @@ -97,8 +108,7 @@ def test_bad_tokens_fail_closed(self): ("garbage", "not.a.jwt"), ]: self._start_state() - resp = self.client.get("/accounts/civil/callback", - {"token": token, "state": "st4te"}) + resp = self.client.get("/accounts/civil/callback", {"token": token, "state": "st4te"}) self.assertIn("/login/", resp["Location"], label) self.assertEqual(CivilIdentity.objects.count(), 0, label) @@ -107,8 +117,9 @@ def test_inactive_local_user_cannot_enter(self): user = get_user_model().objects.create_user("bob", password="x", is_active=False) CivilIdentity.objects.create(user=user, civil_id=sub) self._start_state() - resp = self.client.get("/accounts/civil/callback", - {"token": forge(sub=sub), "state": "st4te"}) + resp = self.client.get( + "/accounts/civil/callback", {"token": forge(sub=sub), "state": "st4te"} + ) self.assertIn("/login/", resp["Location"]) self.assertNotIn("_auth_user_id", self.client.session) @@ -116,10 +127,8 @@ def test_inactive_local_user_cannot_enter(self): class DisabledTests(TestCase): def test_unconfigured_means_404(self): # No CIVIL_URL: the routes simply do not exist for this install. - self.assertEqual( - self.client.get("/accounts/civil/login/").status_code, 404) - self.assertEqual( - self.client.get("/accounts/civil/callback").status_code, 404) + self.assertEqual(self.client.get("/accounts/civil/login/").status_code, 404) + self.assertEqual(self.client.get("/accounts/civil/callback").status_code, 404) @override_settings(CIVIL_URL="http://civil.test") @@ -127,28 +136,28 @@ class LoginStartTests(TestCase): def test_redirects_to_civil_with_state_and_callback(self): resp = self.client.get("/accounts/civil/login/", {"next": "/tickets/"}) self.assertEqual(resp.status_code, 302) - self.assertTrue(resp["Location"].startswith( - "http://civil.test/sso/authorize?")) + self.assertTrue(resp["Location"].startswith("http://civil.test/sso/authorize?")) self.assertIn("app=jackil", resp["Location"]) self.assertIn("state=", resp["Location"]) self.assertEqual(self.client.session["civilsso_next"], "/tickets/") def test_offsite_next_is_dropped(self): - self.client.get("/accounts/civil/login/", - {"next": "https://evil.example.com/"}) + self.client.get("/accounts/civil/login/", {"next": "https://evil.example.com/"}) self.assertNotIn("civilsso_next", self.client.session) class CivilConfigTests(TestCase): def test_db_config_enables_without_env(self): from apps.civilsso import client - from apps.civilsso.models import CivilConfig + admin = get_user_model().objects.create_superuser("cfgadmin_x", password="x") self.client.force_login(admin) self.assertFalse(client.enabled()) - r = self.client.post("/api/v1/civil/settings/", - '{"enabled": true, "url": "http://civil.lan:8100/"}', - content_type="application/json") + r = self.client.post( + "/api/v1/civil/settings/", + '{"enabled": true, "url": "http://civil.lan:8100/"}', + content_type="application/json", + ) self.assertEqual(r.status_code, 200, r.content) self.assertTrue(r.json()["active"]) self.assertEqual(client.civil_url(), "http://civil.lan:8100") diff --git a/apps/civilsso/views.py b/apps/civilsso/views.py index 2ca34ba..62e96c4 100644 --- a/apps/civilsso/views.py +++ b/apps/civilsso/views.py @@ -36,11 +36,13 @@ def login_start(request): nxt = request.GET.get("next", "") if nxt and url_has_allowed_host_and_scheme(nxt, allowed_hosts={request.get_host()}): request.session[_NEXT_SESSION_KEY] = nxt - query = urlencode({ - "app": client.app_slug(), - "redirect_uri": request.build_absolute_uri(reverse("civil-callback")), - "state": state, - }) + query = urlencode( + { + "app": client.app_slug(), + "redirect_uri": request.build_absolute_uri(reverse("civil-callback")), + "state": state, + } + ) return redirect(f"{client.civil_url()}/sso/authorize?{query}") @@ -159,5 +161,6 @@ def civil_settings_page(request): return redirect_to_login(request.get_full_path()) if not (user.is_staff or user.is_superuser): from django.http import HttpResponseForbidden + return HttpResponseForbidden("Administrator access required.") return render(request, "civilsso/settings.html")