fix: gate review LiveView with selfhosted+OAuth auth requirement#145
Merged
tomasz-tomczyk merged 2 commits intomainfrom Apr 30, 2026
Merged
fix: gate review LiveView with selfhosted+OAuth auth requirement#145tomasz-tomczyk merged 2 commits intomainfrom
tomasz-tomczyk merged 2 commits intomainfrom
Conversation
The /r/:token review LiveView did not enforce auth in selfhosted+OAuth mode, even though ApiAuth already gated the JSON API under the same condition. Worse, an existing inline template gate masked the leak — mount/3 still subscribed to PubSub, called touch_last_activity, and push_event "init" with full comments+files to unauthenticated clients. Closes the gap with a new :require_review_auth on_mount hook (mirrors ApiAuth's enforcement condition via a new Crit.Config.selfhosted_oauth?/0 helper that's now the single source of truth for the predicate). On selfhosted+OAuth, unauthenticated visitors are redirected to /auth/login?return_to=<request_path> before mount/3 runs. Public/hosted mode (no SELFHOSTED env) is unchanged — anonymous reviewers and OAuth sign-in continue to work exactly as before. Also moves dev/test-only seed routes (/api/test/seed-user etc.) from the :api pipeline to a separate dev/test-only :device_api scope, so they're reachable for integration tests against an enforcing instance. Production unaffected (these routes only exist in :test/:dev envs). Adds scripts/start-selfhosted.sh for booting a SELFHOSTED+OAuth instance on :4001 with a separate DB, used by the companion crit/ selfhosted integration test suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #145 +/- ##
==========================================
- Coverage 77.25% 77.23% -0.03%
==========================================
Files 52 53 +1
Lines 1574 1577 +3
==========================================
+ Hits 1216 1218 +2
- Misses 358 359 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Previously, the owner check (`current_user && (is_nil(review.user_id) || review.user_id == current_user.id)`) treated anonymous reviews (`user_id == nil`) as owned by any logged-in user — both in the LiveView UI gate and in the server-side `Reviews.delete_review/2` guard. This let any authenticated user delete any anonymous review. Anonymous reviews can only be deleted by their `delete_token` (via `Reviews.delete_by_delete_token/1`), never by an authenticated user. User-owned reviews can only be deleted by the matching user. Also drop the redundant `enforced?/1` wrapper in `ApiAuth` and call `Crit.Config.selfhosted_oauth?/0` directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tomasz-tomczyk
added a commit
that referenced
this pull request
Apr 30, 2026
Post-rebase adjustments after pulling main: - review_live.ex: drop unused current_user binding in mount_review/4 (current_user lookup is no longer needed locally — auth gating runs in the :require_review_scope on_mount hook). - reviews_test.exs: adapt the three tests #145 added that called delete_review(id, owner_id: ...) to the scope-first signature delete_review(scope, id). Also flip the "legacy/anonymous review can be deleted by any authed user" test — main's #145 tightened that guard so anonymous reviews can only be removed via delete_token, and the new scope-side guard matches that semantics (review.user_id != scope.user.id rejects nil user_id). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tomasz-tomczyk
added a commit
that referenced
this pull request
Apr 30, 2026
…ew (#143) * feat: add Crit.Accounts.Scope struct * feat: add CritWeb.UserAuth plug and on_mount hooks * refactor: thread Crit.Accounts.Scope through Reviews and LiveView * docs: add scope pattern reference Project-local rules for the Phoenix 1.8 Scope pattern adopted in Crit.Reviews + LiveView + controllers. Documents argument-order convention, when functions take scope, the display_name rule, the mutual-exclusion invariant, and the migration path for new code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: address scope-refactor code review - Inline `load_user/1` into `Scope.for_session/1` (single call site). - Refactor `resolve_comment` to two simple Repo lookups + private `can_resolve_comment?(scope, comment, review)` helper; drop the joined Repo.one + projection map. - Make `comment_owned_by?/2` take `(scope, comment)` instead of `(comment, identity, user_id)`. - Normalise `update_display_name/2` to return `:ok` for both anon and auth scopes (was asymmetric). - Only persist `display_name` to session for anonymous scopes in `ReviewController.set_name`. - `Layouts.site_header` and `Layouts.dashboard_header` take `current_scope` attr instead of `current_user`; matches phx.gen.auth 1.8 generator output and the local CLAUDE.md scope-pattern rules. - Sweep all 12 header call sites to pass `current_scope={@current_scope}`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: harmonize scope refactor with main's review-LV auth gate (#145) Post-rebase adjustments after pulling main: - review_live.ex: drop unused current_user binding in mount_review/4 (current_user lookup is no longer needed locally — auth gating runs in the :require_review_scope on_mount hook). - reviews_test.exs: adapt the three tests #145 added that called delete_review(id, owner_id: ...) to the scope-first signature delete_review(scope, id). Also flip the "legacy/anonymous review can be deleted by any authed user" test — main's #145 tightened that guard so anonymous reviews can only be removed via delete_token, and the new scope-side guard matches that semantics (review.user_id != scope.user.id rejects nil user_id). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: destructure current_scope in set_display_name handler Matches the convention used by every other handler in this module. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/r/:tokenreview LiveView did not enforce auth in selfhosted+OAuth mode, even thoughApiAuthalready gated the JSON API under the same condition. An existing inline template gate masked the leak —mount/3still subscribed to PubSub, calledtouch_last_activity, andpush_event "init"with full comments+files to unauthenticated clients.:require_review_authon_mount hook redirects unauthenticated visitors to/auth/login?return_to=...beforemount/3runs in selfhosted+OAuth mode.Crit.Config.selfhosted_oauth?/0is now the single source of truth for the predicate (replaces three duplicated checks)./api/test/seed-useretc.) out of the:apipipeline (whereApiAuthblocks them in selfhosted mode) into a:device_apiscope still gated byMix.env() in [:test, :dev].scripts/start-selfhosted.shfor booting a SELFHOSTED instance on :4001 with a separate DB (used by the companion crit/ integration tests).Hosted (non-SELFHOSTED) behavior — unchanged
selfhosted_oauth?/0requires bothselfhosted == trueANDoauth_provider != nil. Hosted instances with onlyoauth_providerset returnfalse→ all gates short-circuit. Anonymous reviewers and OAuth-logged-in users continue to work exactly as before.Review
Test plan
test/crit_web/live/review_live_auth_gate_test.exsasserts the redirect tuple AND thatlast_activity_atis unchanged + no PubSub rebroadcast — provesmount/3did not fire.mix precommitclean: 471 tests, 0 failures, sobelow + deps.audit clean.make test-share-sync-selfhosted.🤖 Generated with Claude Code