From b338064b671e2f758360a21751a858b218336e6f Mon Sep 17 00:00:00 2001 From: Jake Benilov Date: Mon, 17 Mar 2014 23:35:05 -0700 Subject: [PATCH 1/3] Add cancan gem --- Gemfile | 1 + Gemfile.lock | 2 ++ 2 files changed, 3 insertions(+) diff --git a/Gemfile b/Gemfile index faa6ae00..377d0009 100644 --- a/Gemfile +++ b/Gemfile @@ -14,6 +14,7 @@ end gem 'kaminari', '0.14.1' gem 'logstasher', '0.4.8' +gem 'cancan', '1.6.10' group :test do gem 'capybara', '2.1.0' diff --git a/Gemfile.lock b/Gemfile.lock index 04408bab..d6e2c48b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -39,6 +39,7 @@ GEM bootstrap-sass (2.3.2.2) sass (~> 3.2) builder (3.0.4) + cancan (1.6.10) capybara (2.1.0) mime-types (>= 1.16) nokogiri (>= 1.3.3) @@ -229,6 +230,7 @@ PLATFORMS DEPENDENCIES aws-ses bootstrap-sass (= 2.3.2.2) + cancan (= 1.6.10) capybara (= 2.1.0) chosen-rails coffee-rails (~> 3.2.1) From 7aabfbbcec94270b5cff7c7dc283456d434836d6 Mon Sep 17 00:00:00 2001 From: Jake Benilov Date: Tue, 18 Mar 2014 06:38:51 +0000 Subject: [PATCH 2/3] Define user roles (viewer, editor, admin) --- app/models/user.rb | 12 ++++++++++++ test/factories/factories.rb | 8 ++++++++ test/unit/user_test.rb | 26 ++++++++++++++++++++++++++ 3 files changed, 46 insertions(+) diff --git a/app/models/user.rb b/app/models/user.rb index 9b823724..76d390d5 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -18,4 +18,16 @@ class User def self.find_by_uid(uid) where(uid: uid).first end + + def viewer? + has_permission?('signin') + end + + def editor? + has_permission?('editor') || admin? + end + + def admin? + viewer? && has_permission?('admin') + end end diff --git a/test/factories/factories.rb b/test/factories/factories.rb index 3c175eea..86aae560 100644 --- a/test/factories/factories.rb +++ b/test/factories/factories.rb @@ -2,5 +2,13 @@ factory :user do sequence(:name) { |n| "Winston #{n}"} permissions { ["signin"] } + + factory :editor do + permissions { ["signin", "editor"] } + end + + factory :admin do + permissions { ["signin", "editor", "admin"] } + end end end diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index 7ac3a1cb..d616c97b 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -9,4 +9,30 @@ class UserTest < ActiveModel::TestCase end end + context "a normal user" do + should "just be a viewer" do + user = create(:user) + assert user.viewer? + refute user.editor? + refute user.admin? + end + end + + context "an editor" do + should "be a viewer as well" do + editor = create(:editor) + assert editor.viewer? + assert editor.editor? + refute editor.admin? + end + end + + context "an admin" do + should "be both a viewer and an editor" do + admin = create(:admin) + assert admin.viewer? + assert admin.editor? + assert admin.admin? + end + end end From f818b2752f0b30501abb3b83cd1663b24c3686a8 Mon Sep 17 00:00:00 2001 From: Jake Benilov Date: Tue, 18 Mar 2014 06:43:58 +0000 Subject: [PATCH 3/3] Define permissions and restrict actions --- app/controllers/application_controller.rb | 5 ++ app/controllers/bookmarklet_controller.rb | 2 + app/controllers/needs_controller.rb | 13 ++++ app/controllers/notes_controller.rb | 1 + app/helpers/nav_tabs_helper.rb | 4 +- app/models/ability.rb | 14 ++++ app/models/user.rb | 7 ++ app/views/needs/_workflow_buttons.html.erb | 10 ++- app/views/needs/actions.html.erb | 10 ++- app/views/needs/revisions.html.erb | 8 +- test/functional/needs_controller_test.rb | 73 ++++++++++++++++++- test/functional/notes_controller_test.rb | 8 +- test/integration/close_as_duplicate_test.rb | 4 +- test/integration/create_a_need_test.rb | 2 +- test/integration/mark_as_out_of_scope_test.rb | 2 +- test/integration/reopening_needs_test.rb | 5 ++ test/integration/update_a_need_test.rb | 2 +- test/test_helper.rb | 13 +++- test/unit/helpers/nav_tabs_helper_test.rb | 32 ++++++-- 19 files changed, 190 insertions(+), 25 deletions(-) create mode 100644 app/models/ability.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 6b958dcc..0b94e2db 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,10 +1,15 @@ class ApplicationController < ActionController::Base protect_from_forgery + check_authorization rescue_from ActionController::InvalidAuthenticityToken do render text: "Invalid authenticity token", status: 403 end + rescue_from CanCan::AccessDenied do |exception| + redirect_to needs_path, alert: "You do not have permission to perform this action." + end + include GDS::SSO::ControllerMethods before_filter :authenticate_user! diff --git a/app/controllers/bookmarklet_controller.rb b/app/controllers/bookmarklet_controller.rb index 4f047de5..6f6ba9b7 100644 --- a/app/controllers/bookmarklet_controller.rb +++ b/app/controllers/bookmarklet_controller.rb @@ -1,4 +1,6 @@ class BookmarkletController < ApplicationController + skip_authorization_check + def bookmarklet # bookmarklet.html.erb end diff --git a/app/controllers/needs_controller.rb b/app/controllers/needs_controller.rb index 379a52c9..0961de12 100644 --- a/app/controllers/needs_controller.rb +++ b/app/controllers/needs_controller.rb @@ -12,6 +12,7 @@ class Http404 < StandardError end def index + authorize! :index, Need opts = params.slice("organisation_id", "page", "q").select { |k, v| v.present? } @needs = Maslow.need_api.needs(opts) respond_to do |format| @@ -25,18 +26,22 @@ def index end def show + authorize! :read, Need @need = load_need end def actions + authorize! :perform_actions_on, Need @need = load_need end def revisions + authorize! :see_revisions_of, Need @need = load_need end def edit + authorize! :update, Need @need = load_need if @need.duplicate? redirect_to need_url(@need.need_id), @@ -47,10 +52,12 @@ def edit end def new + authorize! :create, Need @need = Need.new({}) end def create + authorize! :create, Need @need = Need.new( prepare_need_params(params) ) add_or_remove_criteria(:new) and return if criteria_params_present? @@ -71,6 +78,7 @@ def create end def update + authorize! :update, Need @need = load_need @need.update(prepare_need_params(params)) @@ -92,6 +100,7 @@ def update end def close_as_duplicate + authorize! :close, Need @need = load_need if @need.duplicate? redirect_to need_url(@need.need_id), @@ -102,6 +111,7 @@ def close_as_duplicate end def closed + authorize! :close, Need @need = load_need @need.duplicate_of = Integer(params["need"]["duplicate_of"]) @@ -123,6 +133,7 @@ def closed end def reopen + authorize! :reopen, Need @need = load_need old_canonical_id = @need.duplicate_of @@ -138,6 +149,7 @@ def reopen end def out_of_scope + authorize! :descope, Need @need = load_need unless @need.in_scope.nil? flash[:error] = "This need has already been marked as out of scope" @@ -147,6 +159,7 @@ def out_of_scope end def descope + authorize! :descope, Need @need = load_need unless @need.in_scope.nil? diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index 1d7be700..e51d6698 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -4,6 +4,7 @@ class NotesController < ApplicationController def create + authorize! :create, Note text = params["notes"]["text"] need_id = params["need_id"] @note = Note.new(text, need_id, current_user) diff --git a/app/helpers/nav_tabs_helper.rb b/app/helpers/nav_tabs_helper.rb index f0846414..0c2f5d6d 100644 --- a/app/helpers/nav_tabs_helper.rb +++ b/app/helpers/nav_tabs_helper.rb @@ -2,8 +2,8 @@ module NavTabsHelper def nav_tabs_for(need) tabs = [] tabs << [ "View", need_path(need) ] - tabs << [ "Edit", edit_need_path(need) ] unless need.duplicate? - tabs << [ "Actions", actions_need_path(need) ] + tabs << [ "Edit", edit_need_path(need) ] if !need.duplicate? && current_user.can?(:update, Need) + tabs << [ "Actions", actions_need_path(need) ] if current_user.can?(:perform_actions_on, Need) tabs << [ "History & Notes", revisions_need_path(need) ] tabs end diff --git a/app/models/ability.rb b/app/models/ability.rb new file mode 100644 index 00000000..2e673b74 --- /dev/null +++ b/app/models/ability.rb @@ -0,0 +1,14 @@ +class Ability + include CanCan::Ability + + def initialize(user) + can [ :read, :index, :see_revisions_of ], Need + + if user.editor? + can [ :create, :update, :close, :reopen, :perform_actions_on ], Need + can :create, Note + end + + can :descope, Need if user.admin? + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 76d390d5..4a59d3a4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,4 +1,5 @@ require "gds-sso/user" +require 'ability' class User include Mongoid::Document @@ -15,6 +16,12 @@ class User attr_accessible :email, :name, :uid, :version, :organisation_slug + delegate :can?, :cannot?, :to => :ability + + def ability + @ability ||= Ability.new(self) + end + def self.find_by_uid(uid) where(uid: uid).first end diff --git a/app/views/needs/_workflow_buttons.html.erb b/app/views/needs/_workflow_buttons.html.erb index 3bd33a43..4480e057 100644 --- a/app/views/needs/_workflow_buttons.html.erb +++ b/app/views/needs/_workflow_buttons.html.erb @@ -1,6 +1,8 @@ -<%= render layout: 'workflow_footer' do %> - <%= link_to "Add a new need", new_need_path, class: "btn btn-primary" %> - <% if defined?(need) && !need.duplicate? %> - <%= link_to "Edit", edit_need_path(need), class: "btn btn-success" %> +<% if current_user.can?(:create, Need) || current_user.can?(:update, Need) %> + <%= render layout: 'workflow_footer' do %> + <%= link_to "Add a new need", new_need_path, class: "btn btn-primary" %> + <% if defined?(need) && !need.duplicate? && current_user.can?(:update, Need) %> + <%= link_to "Edit", edit_need_path(need), class: "btn btn-success" %> + <% end %> <% end %> <% end %> diff --git a/app/views/needs/actions.html.erb b/app/views/needs/actions.html.erb index c8d63141..3f0d5c08 100644 --- a/app/views/needs/actions.html.erb +++ b/app/views/needs/actions.html.erb @@ -8,9 +8,13 @@ <%= render :partial => "need_header" %>
- <%= render partial: "needs/actions/out_of_scope" %> -
- <%= render partial: "needs/actions/duplicate_of" %> + <% if current_user.can?(:descope, Need) %> + <%= render partial: "needs/actions/out_of_scope" %> + <% end %> + <% if current_user.can?(:close, Need) %> +
+ <%= render partial: "needs/actions/duplicate_of" %> + <% end %>
<%= render partial: "workflow_buttons", locals: { need: @need } %> diff --git a/app/views/needs/revisions.html.erb b/app/views/needs/revisions.html.erb index 391a5697..04cabdc6 100644 --- a/app/views/needs/revisions.html.erb +++ b/app/views/needs/revisions.html.erb @@ -15,9 +15,11 @@ <% end %>
-
- <%= render partial: "needs/revisions/new_note_form", locals: { need: @need } %> -
+ <% if current_user.can?(:create, Note) %> +
+ <%= render partial: "needs/revisions/new_note_form", locals: { need: @need } %> +
+ <% end %>
<%= render partial: "needs/revisions/recent_changes", locals: { need: @need } %>
diff --git a/test/functional/needs_controller_test.rb b/test/functional/needs_controller_test.rb index ae0e3d91..3cebc3d8 100644 --- a/test/functional/needs_controller_test.rb +++ b/test/functional/needs_controller_test.rb @@ -5,7 +5,7 @@ class NeedsControllerTest < ActionController::TestCase include GdsApi::TestHelpers::NeedApi setup do - login_as stub_user + login_as_stub_user need_api_has_organisations( "ministry-of-justice" => "Ministry of Justice", "competition-commission" => "Competition Commission" @@ -74,6 +74,9 @@ class NeedsControllerTest < ActionController::TestCase end context "POST create" do + setup do + login_as_stub_editor + end def complete_need_data { @@ -168,6 +171,11 @@ def complete_need_data end end + should "stop viewers from creating needs" do + login_as_stub_user + post(:create, need: complete_need_data) + assert_redirected_to needs_path + end end context "GET show" do @@ -270,6 +278,10 @@ def complete_need_data end context "GET edit" do + setup do + login_as_stub_editor + end + context "given a valid need" do setup do @stub_need = Need.new({ @@ -300,6 +312,12 @@ def complete_need_data get :edit, :id => "coffee" assert_response :not_found end + + should "stop viewers from editing needs" do + login_as_stub_user + get :edit, :id => 100001 + assert_redirected_to needs_path + end end context "PUT update" do @@ -315,6 +333,10 @@ def stub_need Need.new(base_need_fields.merge("id" => 100001), true) # existing need end + setup do + login_as_stub_editor + end + should "404 if need not found" do Need.expects(:find).with(100001).raises(Need::NotFound.new(100001)) put :update, :id => "100001", :need => { :goal => "do things" } @@ -384,9 +406,19 @@ def stub_need assert_response 422 end + + should "stop viewers from updating needs" do + login_as_stub_user + put(:update, id: 100001, need: {}) + assert_redirected_to needs_path + end end context "GET out-of-scope" do + setup do + login_as_stub_admin + end + # For the case when the user has no JS context "given a valid need without a set value for in_scope" do setup do @@ -425,9 +457,19 @@ def stub_need assert_redirected_to need_path(@stub_need) end end + + should "stop editors from descoping needs" do + login_as_stub_editor + get :out_of_scope, id: 100001 + assert_redirected_to needs_path + end end context "PUT descope" do + setup do + login_as_stub_admin + end + context "given a valid need without a set value for in_scope" do setup do @stub_need = Need.new({ @@ -504,9 +546,19 @@ def stub_need assert_response :not_found end + + should "stop editors from descoping needs" do + login_as_stub_editor + put :descope, id: 100001 + assert_redirected_to needs_path + end end context "deleting met_when criteria" do + setup do + login_as_stub_editor + end + should "remove the only value" do need = complete_need_data.merge("met_when" => ["Winning"]) post(:create, { delete_criteria: "0", need: need }) @@ -535,6 +587,7 @@ def stub_need context "PUT closed" do setup do + login_as_stub_editor @need = Need.new(base_need_fields.merge("id" => 100002), true) # duplicate Need.stubs(:find).with(100002).returns(@need) @@ -608,10 +661,19 @@ def stub_need assert_equal "There was a problem closing the need as a duplicate", @controller.flash[:error] assert_response 422 end + + should "stop viewers from marking needs as duplicates" do + login_as_stub_user + put :closed, + :id => "100002", + :need => { :duplicate_of => "100000" } + assert_redirected_to needs_path + end end context "DELETE reopen" do setup do + login_as_stub_editor @need = Need.new(base_need_fields.merge("id" => 100002), true) # duplicate @need.stubs(:artefacts).returns([]) Need.stubs(:find).with(100002).returns(@need) @@ -643,10 +705,18 @@ def stub_need assert_equal "There was a problem reopening the need", @controller.flash[:error] assert_response 422 end + + should "stop viewers from reopening needs" do + login_as_stub_user + delete :reopen, + :id => @need.need_id + assert_redirected_to needs_path + end end context "GET actions" do setup do + login_as_stub_editor @stub_need = Need.new({ "id" => 100001, "role" => "person", @@ -682,6 +752,7 @@ def stub_need context "GET close-as-duplicate" do setup do + login_as_stub_editor @stub_need = Need.new({ "id" => 100001, "role" => "person", diff --git a/test/functional/notes_controller_test.rb b/test/functional/notes_controller_test.rb index b54b8aa9..1a21d871 100644 --- a/test/functional/notes_controller_test.rb +++ b/test/functional/notes_controller_test.rb @@ -5,7 +5,7 @@ class NotesControllerTest < ActionController::TestCase include GdsApi::TestHelpers::NeedApi setup do - login_as stub_user + login_as_stub_editor @note_atts = { "notes" => { "text" => "test" @@ -42,5 +42,11 @@ class NotesControllerTest < ActionController::TestCase assert_equal "Note couldn't be saved: Text can't be blank", @controller.flash[:error] refute @controller.flash[:notice] end + + should "stop viewers from creating notes" do + login_as_stub_user + post :create, @note_atts + assert_redirected_to needs_path + end end end diff --git a/test/integration/close_as_duplicate_test.rb b/test/integration/close_as_duplicate_test.rb index 62849275..b2ba556d 100644 --- a/test/integration/close_as_duplicate_test.rb +++ b/test/integration/close_as_duplicate_test.rb @@ -17,7 +17,7 @@ def need_hash end setup do - login_as(stub_user) + login_as_stub_editor need_api_has_organisations( "committee-on-climate-change" => "Committee on Climate Change", "competition-commission" => "Competition Commission", @@ -94,6 +94,8 @@ def need_hash end should "not be able to edit a closed need" do + login_as_stub_editor + @duplicate.merge!("duplicate_of" => "100001") need_api_has_need(@duplicate) need_api_has_need(@need) diff --git a/test/integration/create_a_need_test.rb b/test/integration/create_a_need_test.rb index ffbe5d01..d6f57b84 100644 --- a/test/integration/create_a_need_test.rb +++ b/test/integration/create_a_need_test.rb @@ -3,7 +3,7 @@ class CreateANeedTest < ActionDispatch::IntegrationTest setup do - login_as(stub_user) + login_as_stub_editor need_api_has_organisations( "committee-on-climate-change" => {"name"=>"Committee on Climate Change", "abbreviation"=>"CCC"}, diff --git a/test/integration/mark_as_out_of_scope_test.rb b/test/integration/mark_as_out_of_scope_test.rb index c881d4a5..72fc45e4 100644 --- a/test/integration/mark_as_out_of_scope_test.rb +++ b/test/integration/mark_as_out_of_scope_test.rb @@ -17,7 +17,7 @@ def need_hash end setup do - login_as(stub_user) + login_as_stub_admin need_api_has_organisations( "committee-on-climate-change" => "Committee on Climate Change", "competition-commission" => "Competition Commission", diff --git a/test/integration/reopening_needs_test.rb b/test/integration/reopening_needs_test.rb index 2cd9f99d..0aadc59d 100644 --- a/test/integration/reopening_needs_test.rb +++ b/test/integration/reopening_needs_test.rb @@ -27,6 +27,10 @@ def need_hash end context "reopening a need that was closed as a duplicate" do + setup do + login_as_stub_editor + end + should "be able to reopen a need" do request_body = { "author" => { @@ -57,6 +61,7 @@ def need_hash end should "not allow descoping the need" do + login_as_stub_admin visit "/needs/100002/actions" # 'Out of scope' functionality is disabled if need is closed diff --git a/test/integration/update_a_need_test.rb b/test/integration/update_a_need_test.rb index f3a9db72..f291e50a 100644 --- a/test/integration/update_a_need_test.rb +++ b/test/integration/update_a_need_test.rb @@ -17,7 +17,7 @@ def need_hash end setup do - login_as(stub_user) + login_as_stub_editor need_api_has_organisations( "committee-on-climate-change" => "Committee on Climate Change", "competition-commission" => "Competition Commission", diff --git a/test/test_helper.rb b/test/test_helper.rb index d4352d76..3e76be70 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -35,7 +35,18 @@ def stub_user end def login_as_stub_user - login_as stub_user + @stub_user = create(:user) + login_as @stub_user + end + + def login_as_stub_editor + @stub_user = create(:editor) + login_as @stub_user + end + + def login_as_stub_admin + @stub_user = create(:admin) + login_as @stub_user end def login_as(user) diff --git a/test/unit/helpers/nav_tabs_helper_test.rb b/test/unit/helpers/nav_tabs_helper_test.rb index 5f6595fe..8cd1a29d 100644 --- a/test/unit/helpers/nav_tabs_helper_test.rb +++ b/test/unit/helpers/nav_tabs_helper_test.rb @@ -3,19 +3,39 @@ class NavTabsHelperTest < ActiveSupport::TestCase include NavTabsHelper include Rails.application.routes.url_helpers + attr_reader :current_user setup do @need = Need.new({"id" => 100001}, true) end - should "include all possible tabs" do - assert_equal ["View", "Edit", "Actions", "History & Notes"], - tab_names_on_needs_page_for(@need) + context "for an editor" do + setup do + @current_user = stub(:user) + @current_user.stubs(:can?).with(:update, Need).returns(true) + @current_user.stubs(:can?).with(:perform_actions_on, Need).returns(true) + end + + should "include all possible tabs" do + assert_equal ["View", "Edit", "Actions", "History & Notes"], + tab_names_on_needs_page_for(@need) + end + + should "not include an Edit link if the need is duplicated" do + @need.duplicate_of = 12345 + refute tab_names_on_needs_page_for(@need).include?("Edit") + end end - should "not include an Edit link if the need is duplicated" do - @need.duplicate_of = 12345 - refute tab_names_on_needs_page_for(@need).include?("Edit") + context "for a viewer" do + setup do + @current_user = stub(:can? => false) + end + + should "not include Edit or Actions links" do + assert_equal ["View", "History & Notes"], + tab_names_on_needs_page_for(@need) + end end private