Add role-based tab visibility, route enforcement, and audit trail #136
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "107-role-based-tabs"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Enforce authentication globally when Keycloak is configured, gate every controller by role, make the bottom nav show only the tabs each role is allowed to see, and add PaperTrail audit logging on all domain models.
Changes
app/controllers/application_controller.rb-- Add globalbefore_action :authenticate_user!(gated onkeycloak_configured?), addrequire_role(*roles, **options)class method withonly/exceptsupport, addvisible_tabshelper with TAB_ROLES mapping, add PaperTrailuser_for_paper_trailwhodunnitapp/controllers/sessions_controller.rb-- Addskip_before_action :authenticate_user!so login/logout routes remain accessibleapp/controllers/client_errors_controller.rb-- Addskip_before_action :authenticate_user!for client error reportingapp/controllers/properties_controller.rb-- Addrequire_role :lead, :admin, :super_adminapp/controllers/work_queue_items_controller.rb-- Addrequire_role :member, :lead, :admin, :super_admin, except: :index; index action shows "need crew access" message for clients instead of redirectingapp/controllers/weeks_controller.rb-- Addrequire_role :admin, :super_adminapp/controllers/uploads_controller.rb-- Addrequire_role :lead, :admin, :super_adminapp/controllers/crew_controller.rb-- New controller withrequire_role :admin, :super_adminand placeholder indexapp/views/crew/index.html.erb-- New placeholder viewapp/views/layouts/application.html.erb-- Role-based bottom nav: Today (slot 1), Week (slot 2), Person (slot 3, always visible), Properties (slot 4), Crew (slot 5). Removed New and Photos tabsapp/views/work_queue_items/index.html.erb-- Add no_access guard for client role at rootapp/assets/stylesheets/application.css-- Add styles for no-access notice and crew placeholderconfig/routes.rb-- Addresources :crew, only: [:index]Gemfile-- Addpaper_trailgemapp/models/property.rb,work_queue_item.rb,upload.rb,service.rb-- Addhas_paper_traildb/migrate/20260607011440_create_versions.rb-- PaperTrail versions tablespec/support/omniauth.rb-- Addsign_in_ashelper for role-based request specsspec/requests/role_access_spec.rb-- 27 specs covering auth redirect, role gating per controller, dev mode bypass, client fallbackspec/requests/crew_spec.rb-- 7 specs for crew controller accessspec/models/paper_trail_spec.rb-- 5 specs for whodunnit tracking and version creationTest Plan
bundle exec rspec-- 141 examples, 0 failures (101 existing + 40 new)Review Checklist
Related Notes
None.
Related
Closes #107
QA Review -- PR #136
Reviewed the full diff (25 files, +573/-19) against issue #107 requirements.
Findings
1. [bug] Redirect loop risk for client role on mutating work queue actions
WorkQueueItemsControllerusesrequire_role :member, :lead, :admin, :super_admin, except: :index. This meanscreate,update,destroy, andreorderare role-gated. When a client fails the role check,require_roleredirects toroot_path, which iswork_queue_items#index. The index then renders the no-access page. This is correct behavior -- no loop occurs because the index action is excluded from role enforcement. Verified this is safe. No action needed.2. [nit]
with_keycloakhelper duplicated across three spec filesrole_access_spec.rb,crew_spec.rb, andpaper_trail_spec.rbeach define identicalwith_keycloak/without_keycloakhelpers. Consider extracting tospec/support/keycloak_helpers.rbto DRY up the test suite. Non-blocking.3. [nit]
paper_trail_spec.rbis inspec/models/but hastype: :requestThe file lives at
spec/models/paper_trail_spec.rbbut declarestype: :request(it usespatchandsign_in_as). Should live atspec/requests/paper_trail_spec.rbfor conventional file placement. Non-blocking -- RSpec uses the explicittype:tag, so it works either way.4. [ok] Session controller not modified
Issue spec says "don't modify sessions_controller.rb." The diff adds only
skip_before_action :authenticate_user!-- this is required for the global auth enforcement to work and does not change auth logic. Acceptable.5. [ok] Role mapping matches spec
Verified TAB_ROLES and require_role declarations match the issue's role-to-tab matrix exactly:
6. [ok] Dev graceful degradation
keycloak_configured?guards bothauthenticate_user!(viaif:) andrequire_role(vianext unless).visible_tabsreturns all tabs when not configured. Verified in specs.7. [ok] PaperTrail integration
has_paper_trailon all four models.user_for_paper_trailreturns username or "anonymous". Migration is standard PaperTrail generator output. Whodunnit specs cover both authenticated and anonymous paths.8. [ok] New/Photos tabs removed, Crew tab added
Layout diff confirms New and Photos tabs are gone. Crew tab added in slot 5 with appropriate SVG icon. Person tab always in slot 3.
Summary
No blocking issues found. Two nits (duplicated test helpers, misplaced spec file) are cleanup items that can be addressed in a follow-up.
VERDICT: APPROVE
PaperTrail object_changes column stores per-field diffs (e.g. {"client_name": ["Old", "New"]}) for readable audit trail instead of requiring snapshot comparison. Nav switches from flex to CSS grid with fixed 5-column slots so Person icon stays centered in slot 3 regardless of how many role-based tabs are visible. Closes #137 scope: discovered during PR #136 review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>