Class: RuboCop::Cop::DevDoc::Auth::CurrentUserBranching

Inherits:
Base
  • Object
show all
Includes:
CurrentUserBranchingHelpers
Defined in:
lib/rubocop/cop/dev_doc/auth/current_user_branching.rb

Overview

Forbid branching on authentication state in page-specific code.

Rationale

In a Rails app using Pundit + Devise, current_user is guaranteed non-nil inside any controller action or view that requires auth — the policy has already denied anonymous visitors. Branching on if current_user or if user_signed_in? inside that code is therefore either dead code (the branch for nil can never fire) or a signal that the developer was unsure whether the page requires auth.

If the page genuinely serves both anonymous and signed-in visitors, the branching should be explicit and kept in shared/layout code, not sprinkled through action bodies and page views.

❌ Authenticated page branching on auth state (branch is dead code)
# app/views/posts/show.json.jbuilder
if current_user
json.actions [:edit, :delete]
end

✔️ Shared layout — branching here is the right place
# app/views/layouts/_nav_bar.html.erb
<% if user_signed_in? %>
<%= render 'profile_menu' %>
<% else %>
<%= link_to 'Log in', new_user_session_path %>
<% end %>

✔️ Genuinely dual-state page — suppress the cop with a reason comment
def new
# rubocop:disable DevDoc/Auth/CurrentUserBranching
# Reason: contact form is intentionally dual-state; pre-fills for signed-in users.
if current_user
  @form.name = current_user.full_name
end
# rubocop:enable DevDoc/Auth/CurrentUserBranching
end

Patterns flagged

  • if current_user / unless current_user (block or modifier) where the body is non-empty and is not a bare return (bare returns are the nil-guard pattern covered by LoadResourceCurrentUserGuard; the exemption covers every nil-test spelling of the guard — return if current_user.nil?, return unless current_user.present?, return if !current_user).
  • if user_signed_in? / unless user_signed_in? (any form).
  • The evasion spellings — all equivalent to the bare form and all measured in the wild once the bare form was policed: current_user.present? / .blank? / .nil?, !current_user, !user_signed_in?, and boolean combinations (current_user && x, user_signed_in? || y).
  • ANY method chain rooted at current_user used as a condition — both current_user&.admin? and plain current_user.admin?. The &. is a confession that the dev expects nil; the plain chain is a role/ownership check that belongs in the policy (see below).
  • Ternaries: current_user ? a : b, user_signed_in? ? a : b.
  • Hash/argument values: authenticated: user_signed_in?.
  • ALIASES: a local/instance variable assigned from current_user in the same file inherits every rule above — @user = current_user followed by if @user.admin? is the same branch laundered, and it flags identically. Only PURE aliases are tracked: every assignment to the name must be an unconditional = current_user (case dispatch counts as unconditional). A conditional assignment or a second source makes the variable a genuinely nilable/general resource holder — branching on it is resource-presence logic, not auth state. Taint is per-file only: an ivar assigned in the controller and read in a view is not tracked — pair this cop with the receiver-agnostic DevDoc/Auth/RolePredicateOutsidePolicy, which closes that gap for the known role predicates.

AllowedMethods — classifying data reads

The first method called on current_user (or a tainted alias) in a condition is CLASSIFIED: entries in AllowedMethods are data reads (current_user.errors.any?, an MFA-configured flag) and pass; everything else — role predicates and methods nobody has classified yet — flags. The list is a closed set on purpose: every new reader used in a condition forces a reviewed "data or permission?" decision in .rubocop.yml rather than slipping through. The universally-generic baseline (errors, id, email, persisted?, new_record? — DEFAULT_ALLOWED_METHODS) is built into the cop and cannot be un-configured; the yml list is purely ADDITIVE, so projects list only their own readers:

DevDoc/Auth/CurrentUserBranching:
AllowedMethods:
  - otp_required_for_login

Moving a decision to the policy — query by PURPOSE

The policy move is only complete when the policy owns the role->behavior rule, not just the role's spelling. Export a decision-shaped query (auto_approve?, initiation_blocked_for_admin?, or an authorize block) and call THAT from page code. A generic role getter (def user_is_admin? = current_user&.admin?) re-opens the scattering: every call site invents its own rule again, one policy door down. If your policies must expose such getters as internal building blocks, list their names in the companion cop's RolePredicates so page code cannot call them.

Allowed paths (Exclude:)

By default the cop is silent in:

app/policies/**/*.rb          ← auth-dependent checks belong here
app/helpers/**/*.rb
app/controllers/concerns/**/*.rb
app/views/layouts/**/*        ← display branching (nav bars, etc.)

Override via Exclude: in your .rubocop.yml. Note: literal file paths (e.g. app/controllers/application_controller.rb) in .rubocop.yml Exclude: lists are flagged by the DevDoc::Test::Lints::NoFileExcludes lint — they hide the suppression from readers of that file. If ApplicationController needs an exception, use an inline # rubocop:disable at the specific line with a reason.

Before disabling inline, consider the Policy

The Policy exclusion is not accidental — it's the canonical Rails home for auth-dependent branching. If the flagged line is an authorization check (return if @record.accessible_by?(current_user), if current_user.admin?, etc.), the right move is almost always to push that check into a Pundit policy method, not disable inline. The controller becomes return unless policy(@record).access? — same behaviour, no current_user reference in the controller, and the auth logic is reusable + testable in isolation.

Even better: declare glib_authorize_resource at the controller level. glib-web runs the appropriate policy method before each action automatically, so the per-action verify_access / authorize @record line is usually not needed at all — the controller body no longer references current_user because the auth check has moved out of the action entirely. See best_practices/backend/en/05_controller.md item #7 for the canonical pattern.

❌ Auth check in the controller — flagged
def verify_access
return if @support_question.accessible_by?(current_user)
# ... token fallback ...
end

✔️ Same check in the Policy — silently allowed
# app/policies/support_question_policy.rb
def access?
user.present? && record.accessible_by?(user)
end

# in the controller:
def verify_access
return if policy(@support_question).access?
# ... token fallback ...
end

NOTE: The cop does not autocorrect — there is no mechanical fix. The right response depends on developer intent: drop the branch (if auth is required), restructure into a shared layout (if dual-state), or add an inline disable with a reason.

Examples:

# bad — inside an authenticated action
def show
  if current_user
    @data = current_user.private_data
  end
end

# bad — safe-nav predicate, same anti-pattern in disguise
def show
  if current_user&.admin?
    admin_thing
  end
end

# bad — inside a page view
# user_signed_in? ? render_private : render_public

# good — bare nil-guard return (LoadResourceCurrentUserGuard rule)
return unless current_user

# good — inside a shared layout file (excluded by default)
# app/views/layouts/_nav.html.erb
if user_signed_in?
  ...
end

Constant Summary collapse

MSG = 
'Branching on auth state in page code — decide what this branch really is: ' \
'auth is required here -> the anonymous branch is dead code, delete it; ' \
'a role/ownership check -> move the DECISION into the Pundit policy and ' \
'query it by purpose (`policy(record).auto_approve?`, `can?(:update, x)`) ' \
'— do NOT export a generic role getter (`user_is_admin?`) from the policy: ' \
'that centralizes only the spelling while every call site keeps its own ' \
'role->behavior rule; a plain data read (not permission-related) -> ' \
'classify the method in AllowedMethods; genuinely dual-state -> move the ' \
'branching to a layout/helper (excluded paths — PRESENTATION branching ' \
'only, keyed on entity-scoped facts, never smuggled permission logic) or ' \
'split anonymous/signed-in partials. Aliasing (`user = current_user`) ' \
'does not exempt any of this. Only a small, local branch on a genuinely ' \
'dual-state PAGE warrants an inline disable with a reason.'.freeze
AUTH_METHODS = 
%i[current_user user_signed_in?].freeze
VALUE_FLAGGED_METHODS =

value: user_signed_in? — the BOOLEAN auth state passed as data, which smuggles the branch to the consumer/client. Deliberately limited to user_signed_in?: bare current_user passed as data (requested_by: current_user, model: current_user, x.user == current_user) is actorship recording or identity comparison, not branching — flagging it measured 44 noise offenses / 0 finds on a mature codebase.

%i[user_signed_in?].freeze

Constants included from CurrentUserBranchingHelpers

RuboCop::Cop::DevDoc::Auth::CurrentUserBranchingHelpers::DEFAULT_ALLOWED_METHODS, RuboCop::Cop::DevDoc::Auth::CurrentUserBranchingHelpers::IDENTITY_COMPARISON_METHODS, RuboCop::Cop::DevDoc::Auth::CurrentUserBranchingHelpers::NIL_TEST_METHODS

Instance Method Summary collapse

Methods included from CurrentUserBranchingHelpers

#on_new_investigation

Instance Method Details

#on_if(node) ⇒ Object 



209
210
211
212
213
214
 
# File 'lib/rubocop/cop/dev_doc/auth/current_user_branching.rb', line 209

def on_if(node)
  return unless auth_branch?(node)
  return if bare_return_guard?(node)

  add_offense(if_offense_location(node))
end

#on_send(node) ⇒ Object 



225
226
227
228
229
230
 
# File 'lib/rubocop/cop/dev_doc/auth/current_user_branching.rb', line 225

def on_send(node)
  return unless VALUE_FLAGGED_METHODS.include?(node.method_name) && node.receiver.nil?
  return unless used_as_value?(node)

  add_offense(node.loc.selector)
end