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 Integration #2399

Merged
merged 5 commits into from
Sep 19, 2015
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ group :test do
gem 'rubocop', '~> 0.31.0'
gem 'simplecov', '>= 0.9', require: false
gem 'timecop', '>= 0.5'

gem 'pundit'
platforms :ruby_21, :ruby_22 do
gem 'refile', '~> 0.5', require: 'refile/rails'
gem 'refile-mini_magick', '>= 0.1.0'
Expand Down
1 change: 1 addition & 0 deletions app/controllers/rails_admin/main_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ class MainController < RailsAdmin::ApplicationController
include ActionView::Helpers::TextHelper
include RailsAdmin::MainHelper
include RailsAdmin::ApplicationHelper
include Pundit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes Pundit required dependency, we should make it optional choice.


layout :get_layout

Expand Down
1 change: 1 addition & 0 deletions lib/rails_admin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
require 'rails_admin/extension'
require 'rails_admin/extensions/cancan'
require 'rails_admin/extensions/cancancan'
require 'rails_admin/extensions/pundit'
require 'rails_admin/extensions/paper_trail'
require 'rails_admin/extensions/history'
require 'rails_admin/support/csv_converter'
Expand Down
3 changes: 3 additions & 0 deletions lib/rails_admin/extensions/pundit.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
require 'rails_admin/extensions/pundit/authorization_adapter'

RailsAdmin.add_extension(:pundit, RailsAdmin::Extensions::Pundit, authorization: true)
60 changes: 60 additions & 0 deletions lib/rails_admin/extensions/pundit/authorization_adapter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
module RailsAdmin
module Extensions
module Pundit
# This adapter is for the Pundit[https://github.com/elabs/pundit] authorization library.
# You can create another adapter for different authorization behavior, just be certain it
# responds to each of the public methods here.
class AuthorizationAdapter
# See the +authorize_with+ config method for where the initialization happens.
def initialize(controller)
@controller = controller
@controller.class.send(:alias_method, :pundit_user, :_current_user)
end

# This method is called in every controller action and should raise an exception
# when the authorization fails. The first argument is the name of the controller
# action as a symbol (:create, :bulk_delete, etc.). The second argument is the
# AbstractModel instance that applies. The third argument is the actual model
# instance if it is available.
def authorize(action, abstract_model = nil, model_object = nil)
record = model_object || abstract_model && abstract_model.model
fail ::Pundit::NotAuthorizedError.new("not allowed to #{action} this #{record}") unless policy(record).rails_admin?(action)
end

# This method is called primarily from the view to determine whether the given user
# has access to perform the action on a given model. It should return true when authorized.
# This takes the same arguments as +authorize+. The difference is that this will
# return a boolean whereas +authorize+ will raise an exception when not authorized.
def authorized?(action, abstract_model = nil, model_object = nil)
record = model_object || abstract_model && abstract_model.model
policy(record).rails_admin?(action)
end

# This is called when needing to scope a database query. It is called within the list
# and bulk_delete/destroy actions and should return a scope which limits the records
# to those which the user can perform the given action on.
def query(_action, abstract_model)
@controller.policy_scope(abstract_model.model.all)
rescue ::Pundit::NotDefinedError
abstract_model.model.all
end

# This is called in the new/create actions to determine the initial attributes for new
# records. It should return a hash of attributes which match what the user
# is authorized to create.
def attributes_for(action, abstract_model)
record = abstract_model && abstract_model.model
policy(record).try(:attributes_for, action) || {}
end

private

def policy(record)
@controller.policy(record)
rescue ::Pundit::NotDefinedError
::ApplicationPolicy.new(@controller.send(:_current_user), record)
end
end
end
end
end
228 changes: 228 additions & 0 deletions spec/integration/authorization/pundit_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,228 @@
require 'spec_helper'

class ApplicationPolicy
attr_reader :user, :record

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

def new?
user.roles.include? :admin
end

def show?
true
end

def update?
user.roles.include? :admin
end

def create?
user.roles.include? :admin
end

def edit?
user.roles.include? :admin
end

def destroy?
user.roles.include? :admin
end

def rails_admin?(action)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that the purpose of this method is to enable different authorization for front-app and RailsAdmin backend, but that's not necessary. Just checking create?, update?... also in RailsAdmin side is enough.

If users need such authorization, they can customize action configuration like:

config.actions do
  new do
    authorization_key :rails_admin_new
  end
  edit do
    authorization_key :rails_admin_edit
  end
  ...
end

In this way, RA will check for rails_admin_new? and rails_admin_edit?, instead of new? and edit?.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this issue?
We really need to get rid of this ugly case-when chain.

case action
when :dashboard
user.roles.include? :admin
when :index
false
when :show
user.roles.include? :admin
when :new
user.roles.include? :admin
when :edit
user.roles.include? :admin
when :destroy
false
when :export
user.roles.include? :admin
when :history
user.roles.include? :admin
when :show_in_app
user.roles.include? :admin
else
fail ::Pundit::NotDefinedError.new("unable to find policy #{action} for #{record}.")
end
end
end

class PlayerPolicy < ApplicationPolicy
def rails_admin?(action)
case action
when :index
user.roles.include? :admin
when :show
true
when :new
(user.roles.include?(:create_player) || user.roles.include?(:admin) || user.roles.include?(:manage_player))
when :edit
(user.roles.include? :manage_player)
when :destroy
(user.roles.include? :manage_player)
when :export
user.roles.include? :admin
when :history
user.roles.include? :admin
when :show_in_app
(user.roles.include?(:admin) || user.roles.include?(:manage_player))
else
fail ::Pundit::NotDefinedError.new("unable to find policy #{action} for #{record}.")
end
end
end

describe PlayerPolicy do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't test behavior of policies themselves here.
What we want to test is integration of RailsAdmin and Pundit, so ensuring pundit's functionality or policy's correctness is not our job.

before do
RailsAdmin.config do |c|
c.authorize_with(:pundit)
c.authenticate_with { warden.authenticate! scope: :user }
c.current_user_method(&:current_user)
end
@user = FactoryGirl.create :user
@player_model = RailsAdmin::AbstractModel.new(Player)
login_as @user
end

subject { PlayerPolicy.new(user, player) }

let(:player) { @player_model }

describe 'for a user with no roles' do
let(:user) { @user }

it { should permit(:show) }
it { should_not permit(:create) }
it { should_not permit(:new) }
it { should_not permit(:update) }
it { should_not permit(:edit) }
it { should_not permit(:destroy) }
end

describe 'for an admin' do
before do
@user.update_attributes(roles: [:admin, :read_player])
end

let(:user) { @user }
it { should permit(:show) }
it { should permit(:create) }
it { should permit(:new) }
it { should permit(:update) }
it { should permit(:edit) }
it { should permit(:destroy) }
end
end

describe 'RailsAdmin Pundit Authorization', type: :request do
subject { page }

before do
RailsAdmin.config do |c|
c.authorize_with(:pundit)
c.authenticate_with { warden.authenticate! scope: :user }
c.current_user_method(&:current_user)
end
@player_model = RailsAdmin::AbstractModel.new(Player)
@user = FactoryGirl.create :user
login_as @user
end

describe 'with no roles' do
before do
@user.update_attributes(roles: [])
end

it 'GET /admin should raise Pundit::NotAuthorizedError' do
expect { visit dashboard_path }.to raise_error(Pundit::NotAuthorizedError)
end

it 'GET /admin/player should raise Pundit::NotAuthorizedError' do
expect { visit index_path(model_name: 'player') }.to raise_error(Pundit::NotAuthorizedError)
end
end

describe 'with read player role' do
before do
@user.update_attributes(roles: [:admin, :read_player])
end

it 'GET /admin should show Player but not League' do
visit dashboard_path
is_expected.to have_content('Player')
is_expected.not_to have_content('League')
is_expected.not_to have_content('Add new')
end

it 'GET /admin/team should raise Pundit::NotAuthorizedError' do
expect { visit index_path(model_name: 'team') }.to raise_error(Pundit::NotAuthorizedError)
end

it 'GET /admin/player/1/edit should raise access denied' do
@player = FactoryGirl.create :player
expect { visit edit_path(model_name: 'player', id: @player.id) }.to raise_error(Pundit::NotAuthorizedError)
end
end

describe 'with admin role' do
before do
@user.update_attributes(roles: [:admin, :manage_player])
end

it 'GET /admin should show Player but not League' do
visit dashboard_path
is_expected.to have_content('Player')
end

it 'GET /admin/player/new should render and create record upon submission' do
visit new_path(model_name: 'player')

is_expected.to have_content('Save and edit')
is_expected.not_to have_content('Delete')

is_expected.to have_content('Save and add another')
fill_in 'player[name]', with: 'Jackie Robinson'
fill_in 'player[number]', with: '42'
fill_in 'player[position]', with: 'Second baseman'
click_button 'Save' # first(:button, "Save").click
is_expected.not_to have_content('Edit')

@player = RailsAdmin::AbstractModel.new('Player').first
expect(@player.name).to eq('Jackie Robinson')
expect(@player.number).to eq(42)
expect(@player.position).to eq('Second baseman')
end
end

describe 'with all roles' do
it 'shows links to all actions' do
@user.update_attributes(roles: [:admin, :manage_player])
@player = FactoryGirl.create :player

visit index_path(model_name: 'player')
is_expected.to have_css('.show_member_link')
is_expected.to have_css('.edit_member_link')
is_expected.to have_css('.delete_member_link')
is_expected.to have_css('.history_show_member_link')
is_expected.to have_css('.show_in_app_member_link')

visit show_path(model_name: 'player', id: @player.id)
is_expected.to have_content('Show')
is_expected.to have_content('Edit')
is_expected.to have_content('Delete')
is_expected.to have_content('History')
is_expected.to have_content('Show in app')
end
end
end
2 changes: 2 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

require 'simplecov'
require 'coveralls'
require 'pundit/rspec'

SimpleCov.formatters = [SimpleCov::Formatter::HTMLFormatter, Coveralls::SimpleCov::Formatter]

Expand All @@ -21,6 +22,7 @@
require 'factory_girl'
require 'factories'
require 'database_cleaner'
require 'support/pundit_matcher.rb'
require "orm/#{CI_ORM}"

Dir[File.expand_path('../shared_examples/**/*.rb', __FILE__)].each { |f| require f }
Expand Down
13 changes: 13 additions & 0 deletions spec/support/pundit_matcher.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
RSpec::Matchers.define :permit do |action|
match do |policy|
policy.public_send("#{action}?")
end

failure_message do |policy|
"#{policy.class} does not permit #{action} on #{policy.record} for #{policy.user.inspect}."
end

failure_message_when_negated do |policy|
"#{policy.class} does not forbid #{action} on #{policy.record} for #{policy.user.inspect}."
end
end