Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pundit policy checks should automatically fail if current_user is missing #483

Open
eoinkelly opened this issue Sep 3, 2023 · 0 comments
Labels
discuss Discussion required good first issue Good for newcomers ready for dev

Comments

@eoinkelly
Copy link
Contributor

I think it would be useful to have our pundit ApplicationPolicy raise an exception if current_user is nil.

The pundit README has an example of this for what it calls "closed systems (systems where you have to be a user to do anything). I think that "closed systems" are the most common kind of app we build so this would be a useful change.

Policies that don't require a current_user are 1) quite rare and 2) can just not inherit from ApplicationPolicy.

I'm happy to make the PR if we have consensus on this being a useful change.

The ApplicationPolicy we currently generate

# frozen_string_literal: true

class ApplicationPolicy
  attr_reader :user, :record

  def initialize(user, record)
    @user = user
    @record = record
  end

  def index?
    false
  end

  def show?
    false
  end

  def create?
    false
  end

  def new?
    create?
  end

  def update?
    false
  end

  def edit?
    update?
  end

  def destroy?
    false
  end

  class Scope
    def initialize(user, scope)
      @user = user
      @scope = scope
    end

    def resolve
      raise NotImplementedError, "You must define #resolve in #{self.class}"
    end

    private

    attr_reader :user, :scope
  end
end

The ApplicationPolicy I am proposing we generate

class ApplicationPolicy
  attr_reader :user, :record

  def initialize(user, record)
    # We should immediately fail if we try to authorize an endpoint which does
    # not already require authentication as per
    # https://github.com/varvet/pundit#closed-systems
    fail Pundit::NotAuthorizedError, "must be logged in" unless user

    @user = user
    @record = record
  end

  def index?
    false
  end

  def show?
    false
  end

  def create?
    false
  end

  def new?
    create?
  end

  def update?
    false
  end

  def edit?
    update?
  end

  def destroy?
    false
  end

  class Scope
    def initialize(user, scope)
      # We should immediately fail if we try to authorize an endpoint which does
      # not already require authentication as per
      # https://github.com/varvet/pundit#closed-systems
      fail Pundit::NotAuthorizedError, "must be logged in" unless user

      @user = user
      @scope = scope
    end

    def resolve
      fail NotImplementedError, "You must define #resolve in #{self.class}"
    end

    private

    attr_reader :user, :scope
  end
end
@eoinkelly eoinkelly added the discuss Discussion required label Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Discussion required good first issue Good for newcomers ready for dev
Projects
None yet
Development

No branches or pull requests

2 participants