feat: add granular permission and group system for conference management#60
feat: add granular permission and group system for conference management#60JacobCoffee merged 12 commits intomainfrom
Conversation
…d add required_permission to all views
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a fine-grained, permission-based access control system for conference management by adding new custom permissions, updating all management views to declare required permissions, rebuilding staff groups around those permissions, and updating the management sidebar to hide inaccessible sections.
Changes:
- Add custom permissions to
Conference(22) andTravelGrant(2) and ship migrations for both. - Replace the legacy “monolithic” management permission mixins with
ConferencePermissionMixin+ per-viewrequired_permission. - Rewrite
setup_groupsto create 10 role-based groups and update tests/templates to use the new model.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_registration/test_bootstrap_registration.py | Updates setup_groups tests for the new 10-group model. |
| tests/test_manage/test_report_views.py | Updates report-view fixtures to grant permissions via the new group/perm model. |
| tests/test_manage/test_financial_views.py | Updates finance fixtures to use new finance/report/registration permissions. |
| tests/test_manage/test_analytics_reports.py | Updates analytics fixtures to use the new report permissions model. |
| src/django_program/programs/models.py | Adds new custom TravelGrant permissions. |
| src/django_program/programs/migrations/0011_alter_travelgrant_options.py | Migration to apply updated TravelGrant permissions. |
| src/django_program/manage/views.py | Introduces ConferencePermissionMixin, sidebar permission context, and adds required_permission across many views. |
| src/django_program/manage/views_vouchers.py | Adds required_permission for voucher bulk generation. |
| src/django_program/manage/views_terminal.py | Adds required_permission for Terminal POS. |
| src/django_program/manage/views_reports.py | Switches report views to permission-based mixin and adds per-view permissions. |
| src/django_program/manage/views_overrides.py | Adds per-view override permissions. |
| src/django_program/manage/views_letters.py | Adds per-view registration/letter permissions. |
| src/django_program/manage/views_financial.py | Switches financial views to the new permission-based mixin and adds required permission. |
| src/django_program/manage/views_checkin.py | Adds per-view check-in permissions. |
| src/django_program/manage/views_bulk_purchases.py | Adds per-view bulk purchase permissions. |
| src/django_program/manage/views_analytics.py | Switches analytics views to the new permission-based mixin and adds required permission. |
| src/django_program/manage/templates/django_program/manage/base.html | Adds permission-gated sidebar visibility and collapsible sidebar UI behavior. |
| src/django_program/conference/models.py | Adds new custom Conference permissions. |
| src/django_program/conference/migrations/0010_alter_conference_options.py | Migration to apply updated Conference permissions. |
| src/django_program/conference/management/commands/setup_groups.py | Replaces old groups with 10 new role groups and updates permission assignment logic. |
Comments suppressed due to low confidence (3)
src/django_program/manage/views_reports.py:126
- Report/analytics sidebar highlighting relies on
active_nav, but report views no longer set it now thatReportPermissionMixin.get_context_data()was removed. As a result, the Reports/Analytics links won’t get theactiveclass and the new “don’t collapse the active section” logic won’t work correctly. Consider settingcontext["active_nav"] = "reports"in the shared base for report views (or inConferencePermissionMixinvia a per-view attribute).
class ReportsDashboardView(ConferencePermissionMixin, TemplateView):
"""Landing page for all admin reports with summary statistics."""
template_name = "django_program/manage/reports_dashboard.html"
required_permission = "view_reports"
def get_context_data(self, **kwargs: object) -> dict[str, object]:
"""Build context with summary stats for all report types.
Args:
**kwargs: Additional context data.
Returns:
Template context with attendee, inventory, voucher, and discount summaries.
"""
context: dict[str, object] = super().get_context_data(**kwargs)
conference = self.conference
src/django_program/manage/templates/django_program/manage/base.html:1272
- The "Sync & Overrides" sidebar section is shown when
user_perms.overrides or user_perms.settings, but the only link in the section is Overrides. Users with settings permission but without overrides will still see an Overrides link that will 403. Consider wrapping the Overrides link/subnav inuser_perms.overrides(and similarly guard any sync links withuser_perms.settingsif/when added here).
{% if user_perms.overrides or user_perms.settings %}
<hr class="sidebar-utility-separator">
<div class="sidebar-section" data-section="sync-overrides">
<div class="sidebar-section-title">Sync & Overrides <svg class="sidebar-collapse-icon" viewBox="0 0 16 16" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round"><path d="M4 6l4 4 4-4"/></svg></div>
<ul class="sidebar-nav">
<li>
<div class="sidebar-nav-expandable">
<a href="{% url 'manage:override-list' conference.slug %}" class="{% if active_nav == 'overrides' %}active{% endif %}">
<span class="sidebar-nav-icon"><svg width="16" height="16" fill="none" stroke="currentColor" stroke-width="1.5"><path d="M11 1.5L4.5 8l2 2L13 3.5"/><path d="M2.5 12.5h11M2.5 14.5h7"/></svg></span> Overrides
</a>
src/django_program/conference/management/commands/setup_groups.py:170
setup_groupssilently ignores any permission codenames that aren’t found (it just sets whatevermatchedreturns). With many new custom permissions, a typo or missing migration could leave groups under-permissioned without any signal. Consider detecting and reporting/raising on missing permissions (e.g., computemissing = perm_set - foundand print a warning or fail the command whenmissingis non-empty).
perm_set = set(perm_specs)
app_labels = {app for app, _ in perm_set}
permissions = Permission.objects.filter(
content_type__app_label__in=app_labels,
)
matched = [p for p in permissions if (p.content_type.app_label, p.codename) in perm_set]
group.permissions.set(matched)
if verbosity > 0:
self.stdout.write(self.style.SUCCESS(f" {verb} group '{group_name}' with {len(matched)} permissions"))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/django_program/manage/templates/django_program/manage/base.html
Outdated
Show resolved
Hide resolved
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a granular permission + group-based access control system for conference management, replacing the previous “staff/change_conference” gating with per-view required_permission checks and permission-aware sidebar rendering.
Changes:
- Added custom
Conferencepermissions (dashboard/settings/program/registration/commerce/etc.) and expandedTravelGrantpermissions. - Replaced the legacy
ManagePermissionMixin/report/finance mixins withConferencePermissionMixinand appliedrequired_permissionacross management views. - Reworked
setup_groupsto create 10 role-based groups and updated tests/fixtures to use the new permission model.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_registration/test_bootstrap_registration.py | Updates group name expectations and read-only permission assertions for new groups/permissions. |
| tests/test_manage/test_views.py | Adjusts the staff_user fixture to grant a specific management permission. |
| tests/test_manage/test_report_views.py | Updates report fixture to use the new Reports Viewer group and permission-based access. |
| tests/test_manage/test_financial_views.py | Updates finance fixture to the new Finance Team group and new conference permission codenames. |
| tests/test_manage/test_analytics_reports.py | Updates analytics fixture to use Reports Viewer + view_reports permission. |
| src/django_program/registration/views_checkin.py | Expands check-in API authorization to include new check-in/terminal permissions. |
| src/django_program/programs/models.py | Adds new TravelGrant permissions (view_travel_grant, disburse_travel_grant). |
| src/django_program/programs/migrations/0011_alter_travelgrant_options.py | Migration to apply new TravelGrant model permissions. |
| src/django_program/manage/views.py | Introduces ConferencePermissionMixin, sidebar permission context, and applies required_permission to many views. |
| src/django_program/manage/views_vouchers.py | Adds required_permission for voucher bulk generation. |
| src/django_program/manage/views_terminal.py | Adds required_permission for terminal POS view. |
| src/django_program/manage/views_reports.py | Switches reports to ConferencePermissionMixin and enforces view_reports/export_reports permissions. |
| src/django_program/manage/views_overrides.py | Adds per-view override permissions (view_overrides/change_overrides). |
| src/django_program/manage/views_letters.py | Adds per-view registration permissions; tightens download permission due to PII. |
| src/django_program/manage/views_financial.py | Switches financial dashboard to ConferencePermissionMixin with view_finance permission. |
| src/django_program/manage/views_checkin.py | Adds required_permission for check-in management views. |
| src/django_program/manage/views_bulk_purchases.py | Adds required_permission for bulk purchase views and actions. |
| src/django_program/manage/views_analytics.py | Switches analytics views to ConferencePermissionMixin gated by view_reports. |
| src/django_program/manage/templates/django_program/manage/base.html | Adds permission-based sidebar visibility and collapsible sidebar sections. |
| src/django_program/conference/models.py | Adds 22 custom Conference permissions in Meta.permissions. |
| src/django_program/conference/migrations/0010_alter_conference_options.py | Migration to apply new Conference model permissions. |
| src/django_program/conference/management/commands/setup_groups.py | Rewrites group setup into 10 roles and assigns new permission sets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/django_program/manage/templates/django_program/manage/base.html
Outdated
Show resolved
Hide resolved
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ions # Conflicts: # src/django_program/manage/templates/django_program/manage/base.html
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1168,13 +1182,17 @@ | |||
| <span class="sidebar-nav-icon"><svg width="16" height="16" fill="none" stroke="currentColor" stroke-width="1.5"><path d="M3 2.5h10v11H3z"/><path d="M5.5 5.5h5M5.5 7.5h5M5.5 9.5h3"/><path d="M10 1v3M6 1v3"/></svg></span> Visa Letters | |||
| </a> | |||
| </li> | |||
| {% endif %} | |||
| {% if user_perms.badges %} | |||
| <li> | |||
| <a href="{% url 'manage:badge-list' conference.slug %}" class="{% if active_nav == 'badges' %}active{% endif %}"> | |||
| <span class="sidebar-nav-icon"><svg width="16" height="16" fill="none" stroke="currentColor" stroke-width="1.5"><rect x="2" y="3" width="12" height="10" rx="1.5"/><circle cx="7" cy="7" r="1.5"/><path d="M9.5 10V6.5h1"/><path d="M12 10V8.5"/></svg></span> Badges | |||
| </a> | |||
| </li> | |||
| </ul> | |||
| </div> | |||
| {% endif %} | |||
There was a problem hiding this comment.
The Registration sidebar subsection template tags are mis-nested: the {% endif %} for user_perms.registration_people closes before the corresponding <ul>/<div> are closed, but those closing tags are still rendered unconditionally. If user_perms.registration_people is false (but user_perms.badges is true), this will emit stray </ul></div> and break the sidebar markup. Move the endif to wrap the entire People subsection block (including its closing tags), and ensure the Badges link is rendered inside a valid <ul> when shown.
Summary
ManagePermissionMixinwithConferencePermissionMixinthat checks per-viewrequired_permissionattributes, with fallback to superuser and legacychange_conferenceaccessrequired_permissionto all 70+ management views across 10 view modules (views.py, views_financial.py, views_reports.py, views_analytics.py, views_overrides.py, views_checkin.py, views_terminal.py, views_letters.py, views_vouchers.py, views_bulk_purchases.py)setup_groupsmanagement command with 10 groups: Conference Organizer, Program Committee, Registration Manager, Finance Team, Travel Grant Reviewer, Sponsor Manager, Check-in Staff, Activity Organizer, Reports Viewer, Read-Only StaffTest plan
setup_groupscreates all 10 groups with correct permission countschange_conferenceholders still have full accessGenerated with Claude Code