From d00277c978f5ee961b5b05ec340e6c61f8575842 Mon Sep 17 00:00:00 2001 From: F3PiX Date: Tue, 15 May 2018 15:15:17 +0200 Subject: [PATCH 01/20] WIP Rearrange, not change the abilities in Ability, cf recommended best practices This is the work doc. Reset to here before continuing --- app/models/ability.rb | 210 ++++++++++++++++++++++-------------- spec/models/ability_spec.rb | 21 ++-- 2 files changed, 145 insertions(+), 86 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 9d1d22657..c898119ab 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -10,87 +10,141 @@ def initialize(user) alias_action :create, :read, :update, :destroy, to: :crud - can :crud, User, id: user.id - can :crud, User if user.admin? + # unconfirmed user + can :read, User + can :update, User, id: user.id can :resend_confirmation_instruction, User, id: user.id - can :resend_confirmation_instruction, User if user.admin? - - # visibility of email address in user profile - can :read_email, User, id: user.id if !user.hide_email? - can :read_email, User if user.admin? - can :read_email, User do |other_user| - user.confirmed? && (supervises?(other_user, user) || !other_user.hide_email?) - end - - can :crud, Team do |team| - user.admin? || signed_in?(user) && team.new_record? || on_team?(user, team) - end - - can :update_conference_preferences, Team do |team| - team.accepted? && team.students.include?(user) - end - - can :see_offered_conferences, Team do |team| - user.admin? || team.students.include?(user) || team.supervisors.include?(user) - end - - can :accept_or_reject_conference_offer, Team do |team| - team.students.include?(user) - end - - cannot :create, Team do |team| - on_team_for_season?(user, team.season) || !user.confirmed? - end - - can :join, Team do |team| - team.helpdesk_team? and signed_in?(user) and user.confirmed? and not on_team?(user, team) - end - - can :crud, Role do |role| - user.admin? || on_team?(user, role.team) - end - - can :crud, Source do |repo| - user.admin? || on_team?(user, repo.team) - end - - can :supervise, Team do |team| - user.roles.organizer.any? || team.supervisors.include?(user) - end - - can :crud, ConferencePreference do |preference| - user.admin? || (preference.team.students.include? user) - end - - can :crud, Conference if user.admin? || user.current_student? - - # todo add mailing controller and view for users in their namespace, where applicable - can :read, Mailing do |mailing| - mailing.recipient? user - end - - can :crud, :comments if user.admin? - can :read, :users_info if user.admin? || user.supervisor? - - # projects - can :crud, Project do |project| - user.admin? || - (user.confirmed? && user == project.submitter) - end - can :use_as_template, Project do |project| - user == project.submitter && !project.season&.current? - end - - can :create, Project if user.confirmed? - cannot :create, Project if !user.confirmed? - - # activities + can :read_email, User, hide_email: false # view helper + can :read, Team + can :read, Project can :read, :feed_entry - can :read, :mailing if signed_in?(user) - # applications - can :create, :application_draft if user.student? && user.application_drafts.in_current_season.none? - end + # confirmed user + if user.confirmed? + can :crud, User, id: user.id + can :resend_confirmation_instruction, User, id: user.id + can :read, :mailing if signed_in?(user) + # TODO is this solid? || refactor + can :read, Mailing do |mailing| + mailing.recipient? user + end + can :create, Project if user.confirmed? + + # current_student + can :crud, Conference if user.current_student? + + # team member + + # supervisor + can :read, :users_info if user.supervisor? + # CHECK is this solid? + can :read_email, User do |other_user| + user.confirmed? && (supervises?(other_user, user) || !other_user.hide_email?) + end + + + # admin + if user.admin? + can :manage, :all + cannot :create, User # this only happens through GitHub + # only add what they cannot; the following should be redundant + # can [:read, :update, :destroy], User if user.admin? + # can :manage, User if user.admin? #including resending ?? check + # can :resend_confirmation_instruction, User if user.admin? + can :read_email, User if user.admin? # even when user marked email hidden # view helper #Todo check + # can :read, :users_info if user.admin? + # can :crud, Conference if user.admin? + # can :crud, :comments if user.admin? # TODO make this work for associations + end + + + ### please don't read below this line - it's a mess + ################# OLD FILE, # = moved to or rewritten above ############# + + # can :crud, User, id: user.id + # can :crud, User if user.admin? + # can :resend_confirmation_instruction, User, id: user.id + # can :resend_confirmation_instruction, User if user.admin? + + + # visibility of email address in user profile + # can :read_email, User, id: user.id if !user.hide_email? + # can :read_email, User if user.admin? + # Refactor note: split these over abilities + # can :read_email, User do |other_user| + # user.confirmed? && (supervises?(other_user, user) || !other_user.hide_email?) + # end + + can :crud, Team do |team| + user.admin? || signed_in?(user) && team.new_record? || on_team?(user, team) + end + + can :update_conference_preferences, Team do |team| + team.accepted? && team.students.include?(user) + end + + can :see_offered_conferences, Team do |team| + user.admin? || team.students.include?(user) || team.supervisors.include?(user) + end + + can :accept_or_reject_conference_offer, Team do |team| + team.students.include?(user) + end + + cannot :create, Team do |team| + on_team_for_season?(user, team.season) || !user.confirmed? + end + + can :join, Team do |team| + team.helpdesk_team? and signed_in?(user) and user.confirmed? and not on_team?(user, team) + end + + can :crud, Role do |role| + user.admin? || on_team?(user, role.team) + end + + can :crud, Source do |repo| + user.admin? || on_team?(user, repo.team) + end + + can :supervise, Team do |team| + user.roles.organizer.any? || team.supervisors.include?(user) + end + + can :crud, ConferencePreference do |preference| + user.admin? || (preference.team.students.include? user) + end + + # can :crud, Conference if user.admin? || user.current_student? + + # todo add mailing controller and view for users in their namespace, where applicable + # can :read, Mailing do |mailing| + # mailing.recipient? user + # end + + # can :crud, :comments if user.admin? + # can :read, :users_info if user.admin? || user.supervisor? + + # projects + can :crud, Project do |project| + user.admin? || + (user.confirmed? && user == project.submitter) + end + can :use_as_template, Project do |project| + user == project.submitter && !project.season&.current? + end + + # can :create, Project if user.confirmed? + # cannot :create, Project if !user.confirmed? # not copied over, same as the one before + + # activities + # can :read, :feed_entry + # can :read, :mailing if signed_in?(user) + + # applications + can :create, :application_draft if user.student? && user.application_drafts.in_current_season.none? + end # confirmed? + end # initializer def signed_in?(user) user.persisted? diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 62506637d..9a9cd2070 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -19,17 +19,21 @@ context 'when a user is admin' do let(:organizer_role) { create(:organizer_role, user: user) } + before { allow(organizer_role).to receive(:admin?).and_return(true) } + it "should be able to CRUD on anyone's account" do expect(subject).to be_able_to(:crud, organizer_role) end - end - describe 'she/he is not allowed to CRUD on someone else account' do - let(:other_user) { create(:user) } - it { expect(ability).not_to be_able_to(:show, other_user) } + describe 'she/he is not allowed to CRUD on someone else account' do + let(:other_user) { create(:user) } + # But an admin should! show and crud + xit { expect(ability).not_to be_able_to(:show, other_user) } + end end + describe 'who is allowed to see email address in user profile' do # email address is hidden: admin, user's supervisor in current season (confirmed) @@ -92,7 +96,6 @@ end end end - end describe 'who is disallowed to see email address in user profile' do @@ -121,7 +124,8 @@ allow(user).to receive(:admin?).and_return(false) allow(user).to receive(:confirmed?).and_return(false) end - it 'disallows to see not hidden email address' do + # NOTE / TODO is this testing "can? read_email" properly? + xit 'disallows to see not hidden email address' do other_user.hide_email = false expect(ability).not_to be_able_to(:read_email, other_user) end @@ -171,6 +175,8 @@ end end + # i am here + describe "just orga members, team's supervisor and team's students should be able to see offered conference for a team" do let(:user) { build(:student)} @@ -369,7 +375,6 @@ end context 'create' do - it 'can be created if I am confirmed' do expect(subject).to be_able_to :create, Project.new end @@ -377,7 +382,7 @@ it 'cannot be created if I am not confirmed' do user.confirmed_at = nil user.save - expect(subject).not_to be_able_to :create, Project.new + expect(subject).not_to be_able_to :create, Project end end From 27ead4dc942eca36c7b1e3de5658da3e140a4df1 Mon Sep 17 00:00:00 2001 From: F3PiX Date: Tue, 15 May 2018 16:25:58 +0200 Subject: [PATCH 02/20] Clean file, without todo's and check comments --- app/models/ability.rb | 202 ++++++++++++++---------------------- spec/models/ability_spec.rb | 8 +- 2 files changed, 79 insertions(+), 131 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index c898119ab..d0c46f4b8 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -1,6 +1,4 @@ # frozen_string_literal: true -# See the wiki for details: -# https://github.com/ryanb/cancan/wiki/Defining-Abilities class Ability include CanCan::Ability @@ -20,130 +18,82 @@ def initialize(user) can :read, :feed_entry # confirmed user - if user.confirmed? - can :crud, User, id: user.id - can :resend_confirmation_instruction, User, id: user.id - can :read, :mailing if signed_in?(user) - # TODO is this solid? || refactor - can :read, Mailing do |mailing| - mailing.recipient? user - end - can :create, Project if user.confirmed? - - # current_student - can :crud, Conference if user.current_student? - - # team member - - # supervisor - can :read, :users_info if user.supervisor? - # CHECK is this solid? - can :read_email, User do |other_user| - user.confirmed? && (supervises?(other_user, user) || !other_user.hide_email?) - end - - - # admin - if user.admin? - can :manage, :all - cannot :create, User # this only happens through GitHub - # only add what they cannot; the following should be redundant - # can [:read, :update, :destroy], User if user.admin? - # can :manage, User if user.admin? #including resending ?? check - # can :resend_confirmation_instruction, User if user.admin? - can :read_email, User if user.admin? # even when user marked email hidden # view helper #Todo check - # can :read, :users_info if user.admin? - # can :crud, Conference if user.admin? - # can :crud, :comments if user.admin? # TODO make this work for associations - end - - - ### please don't read below this line - it's a mess - ################# OLD FILE, # = moved to or rewritten above ############# - - # can :crud, User, id: user.id - # can :crud, User if user.admin? - # can :resend_confirmation_instruction, User, id: user.id - # can :resend_confirmation_instruction, User if user.admin? - - - # visibility of email address in user profile - # can :read_email, User, id: user.id if !user.hide_email? - # can :read_email, User if user.admin? - # Refactor note: split these over abilities - # can :read_email, User do |other_user| - # user.confirmed? && (supervises?(other_user, user) || !other_user.hide_email?) - # end - - can :crud, Team do |team| - user.admin? || signed_in?(user) && team.new_record? || on_team?(user, team) - end - - can :update_conference_preferences, Team do |team| - team.accepted? && team.students.include?(user) - end - - can :see_offered_conferences, Team do |team| - user.admin? || team.students.include?(user) || team.supervisors.include?(user) - end - - can :accept_or_reject_conference_offer, Team do |team| - team.students.include?(user) - end - - cannot :create, Team do |team| - on_team_for_season?(user, team.season) || !user.confirmed? - end - - can :join, Team do |team| - team.helpdesk_team? and signed_in?(user) and user.confirmed? and not on_team?(user, team) - end - - can :crud, Role do |role| - user.admin? || on_team?(user, role.team) - end - - can :crud, Source do |repo| - user.admin? || on_team?(user, repo.team) - end - - can :supervise, Team do |team| - user.roles.organizer.any? || team.supervisors.include?(user) - end - - can :crud, ConferencePreference do |preference| - user.admin? || (preference.team.students.include? user) - end - - # can :crud, Conference if user.admin? || user.current_student? - - # todo add mailing controller and view for users in their namespace, where applicable - # can :read, Mailing do |mailing| - # mailing.recipient? user - # end - - # can :crud, :comments if user.admin? - # can :read, :users_info if user.admin? || user.supervisor? - - # projects - can :crud, Project do |project| - user.admin? || - (user.confirmed? && user == project.submitter) - end - can :use_as_template, Project do |project| - user == project.submitter && !project.season&.current? - end - - # can :create, Project if user.confirmed? - # cannot :create, Project if !user.confirmed? # not copied over, same as the one before - - # activities - # can :read, :feed_entry - # can :read, :mailing if signed_in?(user) - - # applications - can :create, :application_draft if user.student? && user.application_drafts.in_current_season.none? - end # confirmed? + can :crud, User, id: user.id + can :resend_confirmation_instruction, User, id: user.id + can :read, :mailing if signed_in?(user) + can :read, Mailing do |mailing| + mailing.recipient? user + end + can :create, Project if user.confirmed? + + # current_student + can :crud, Conference if user.current_student? + + # supervisor + can :read, :users_info if user.supervisor? + can :read_email, User do |other_user| + user.confirmed? && (supervises?(other_user, user) || !other_user.hide_email?) + end + + # project submitter + can :crud, Project, submitter_id: user.id if user.confirmed? + can :use_as_template, Project do |project| + user == project.submitter && !project.season&.current? + end + + # admin + if user.admin? + can :manage, :all + can :read_email, User if user.admin? # even when user marked email hidden # view helper + # add cannot's only; after this line + cannot :create, User # this only happens through GitHub + end + + ################# OLD FILE, # = moved to or rewritten above ############ + # NOT everything moved yet # + + can :crud, Team do |team| + user.admin? || signed_in?(user) && team.new_record? || on_team?(user, team) + end + + can :update_conference_preferences, Team do |team| + team.accepted? && team.students.include?(user) + end + + can :see_offered_conferences, Team do |team| + user.admin? || team.students.include?(user) || team.supervisors.include?(user) + end + + can :accept_or_reject_conference_offer, Team do |team| + team.students.include?(user) + end + + cannot :create, Team do |team| + on_team_for_season?(user, team.season) || !user.confirmed? + end + + can :join, Team do |team| + team.helpdesk_team? and signed_in?(user) and user.confirmed? and not on_team?(user, team) + end + + can :crud, Role do |role| + user.admin? || on_team?(user, role.team) + end + + can :crud, Source do |repo| + user.admin? || on_team?(user, repo.team) + end + + can :supervise, Team do |team| + user.roles.organizer.any? || team.supervisors.include?(user) + end + + can :crud, ConferencePreference do |preference| + user.admin? || (preference.team.students.include? user) + end + + # applications + can :create, :application_draft if user.student? && user.application_drafts.in_current_season.none? end # initializer def signed_in?(user) diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 9a9cd2070..62672fd6a 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -124,8 +124,8 @@ allow(user).to receive(:admin?).and_return(false) allow(user).to receive(:confirmed?).and_return(false) end - # NOTE / TODO is this testing "can? read_email" properly? - xit 'disallows to see not hidden email address' do + # NOTE / TODO is this testing "can? read_email" properly? + xit 'disallows to see not hidden email address' do other_user.hide_email = false expect(ability).not_to be_able_to(:read_email, other_user) end @@ -175,8 +175,6 @@ end end - # i am here - describe "just orga members, team's supervisor and team's students should be able to see offered conference for a team" do let(:user) { build(:student)} @@ -382,7 +380,7 @@ it 'cannot be created if I am not confirmed' do user.confirmed_at = nil user.save - expect(subject).not_to be_able_to :create, Project + expect(subject).not_to be_able_to :create, Project.new end end From e1b56f1912232428346c58c8fd07ce575a31e614 Mon Sep 17 00:00:00 2001 From: F3PiX Date: Tue, 15 May 2018 21:33:39 +0200 Subject: [PATCH 03/20] First examples of the New And Complete ability specs. Now with updated abilities to meet specs. --- app/models/ability.rb | 7 ++- spec/factories/users.rb | 6 +- spec/models/ability_spec.rb | 67 ++++++++++++++++++++++- spec/support/shared_examples/abilities.rb | 44 +++++++++++++++ 4 files changed, 119 insertions(+), 5 deletions(-) create mode 100644 spec/support/shared_examples/abilities.rb diff --git a/app/models/ability.rb b/app/models/ability.rb index d0c46f4b8..ede005c2d 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -7,18 +7,21 @@ def initialize(user) user ||= User.new alias_action :create, :read, :update, :destroy, to: :crud + # guest user + can :read, [User, Team, Project, Activity] # unconfirmed user can :read, User can :update, User, id: user.id can :resend_confirmation_instruction, User, id: user.id can :read_email, User, hide_email: false # view helper + can :read, Activity can :read, Team can :read, Project - can :read, :feed_entry + can :read, :feed_entry # check: # confirmed user - can :crud, User, id: user.id + can [:update, :destroy], User, id: user.id can :resend_confirmation_instruction, User, id: user.id can :read, :mailing if signed_in?(user) can :read, Mailing do |mailing| diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 014b4901f..2379360f5 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -1,6 +1,6 @@ FactoryBot.define do factory :user, aliases: [:member] do - github_handle { FFaker::InternetSE.user_name_variant_short } + github_handle { FFaker::InternetSE.unique.user_name_variant_short } name { FFaker::Name.name } email { FFaker::Internet.email } location { FFaker::Address.city } @@ -84,5 +84,9 @@ create(:reviewer_role, user: user) end end + + trait :unconfirmed do + confirmed_at { nil } + end end end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 62672fd6a..1df829d9a 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -2,8 +2,71 @@ require 'cancan/matchers' RSpec.describe Ability, type: :model do - subject { ability } - let(:ability) { Ability.new(user) } + subject(:ability) { Ability.new(user) } + let(:user){ nil } + let(:other_user) { create(:user) } + + public_pages = [Activity, Team, Project ].freeze + user_pages = [User] + restricted_pages = [Application, ApplicationDraft] # todo + + # NOTE FOR REVIEW : WIP, added for illustration + + # The specs follow the sequence from the Ability class + # I added a few specs, especially for User class for different users, + # as an example of the new set up + # The shared examples will move to spec/support. + # + + + describe "Guest User" do + it_behaves_like "can read public pages" + it_behaves_like "can not create new user" + it_behaves_like "has no access to other user's accounts" + it "can not modify anything" do + public_pages.each do |page| + expect(subject).not_to be_able_to([:create, :update, :destroy], page) + end + end + it "can not even modify their own account" do + true # pro memorie + end + it "can not read restricted pages" do + expect(subject).not_to be_able_to(:read, ApplicationDraft) + end + end + + describe "Logged in user, not confirmed" do + let(:user) { create(:user, :unconfirmed) } + + it_behaves_like "can not create new user" + it_behaves_like "has no access to other user's accounts" + it_behaves_like "can modify own account" + it_behaves_like "can read public pages" + it_behaves_like "can do more stuff when logged in" + end + + describe "Confirmed user" do + let(:user) { create(:user) } # default: confirmed + it_behaves_like "can not create new user" + it_behaves_like "has no access to other user's accounts" + it_behaves_like "can modify own account" + it_behaves_like "can read public pages" + it_behaves_like "can do more stuff when logged in" + + # TODO + # it_behaves_like "team member" + # etc + end + + describe "Admin" do + it_behaves_like "can not create new user" + # it_behaves_like "has access to almost everything else" + end + + + + ############# OLD SPECS ################ context 'ability' do context 'when a user is connected' do diff --git a/spec/support/shared_examples/abilities.rb b/spec/support/shared_examples/abilities.rb new file mode 100644 index 000000000..9d2def7f5 --- /dev/null +++ b/spec/support/shared_examples/abilities.rb @@ -0,0 +1,44 @@ +# TODO find a place to put the arrays once, not here and in the spec +public_pages = [Activity, Team, Project ].freeze +user_pages = [User] +restricted_pages = [Application, ApplicationDraft] # todo + +shared_examples 'can read public pages' do + it "has access to public pages" do + public_pages.each do |page| + expect(subject).to be_able_to(:read, page) + end + end + it {expect(subject).to be_able_to(:read, User) } +end + +shared_examples "has access to restricted pages" do + restricted_pages.each do |page| + expect(subject).to be_able_to(:read, page) + end +end + +# Shared examples for User +shared_examples "has no access to other user's accounts" do # pro memorie: outside their team + it { expect(subject).not_to be_able_to([:update, :destroy], other_user) } +end + +shared_examples "can modify own account" do + it { expect(subject).to be_able_to([:update, :destroy], user) } +end + +shared_examples 'can not create new user' do + it { expect(subject).not_to be_able_to(:create, User.new) } +end + +# Shared examples for logged in users +shared_examples "can do more stuff when logged in" do + it 'can add comments' do # + pending "todo" + raise "NotImplementedFakeError" + end + # todo and more +end + +# Todo add shared_examples for other roles etc + From 6b4664ef62463fbff2b2db66a5acdbd2c4bfb22f Mon Sep 17 00:00:00 2001 From: F3PiX Date: Wed, 16 May 2018 23:30:21 +0200 Subject: [PATCH 04/20] Halfway --- app/controllers/conferences_controller.rb | 4 +- app/models/ability.rb | 62 +++++++--- spec/factories/projects.rb | 4 + spec/models/ability_spec.rb | 141 ++++++++++++++++------ spec/support/shared_examples/abilities.rb | 97 +++++++++++---- 5 files changed, 228 insertions(+), 80 deletions(-) diff --git a/app/controllers/conferences_controller.rb b/app/controllers/conferences_controller.rb index b6da69f58..7602599b3 100644 --- a/app/controllers/conferences_controller.rb +++ b/app/controllers/conferences_controller.rb @@ -13,7 +13,9 @@ def show end def create - page_to_redirect = current_user.admin? ? conferences_path : edit_team_path(current_student.current_team) + byebug + # page_to_redirect = current_user.admin? ? conferences_path : edit_team_path(current_student.current_team) + page_to_redirect = conferences_path @conference = build_conference respond_to do |format| diff --git a/app/models/ability.rb b/app/models/ability.rb index ede005c2d..fe1d7e565 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -3,6 +3,11 @@ class Ability include CanCan::Ability + USER_PAGES = [User, Team] #todo + PUBLIC_PAGES = [Activity, Team, Project, Conference].freeze + LOGGED_IN_PAGES = [Comment] # now: create comments #todo + APPLICATION_PAGES = [Application, ApplicationDraft] # todo + def initialize(user) user ||= User.new @@ -10,32 +15,51 @@ def initialize(user) # guest user can :read, [User, Team, Project, Activity] - # unconfirmed user - can :read, User + # unconfirmed, logged in user + can :read, USER_PAGES can :update, User, id: user.id can :resend_confirmation_instruction, User, id: user.id - can :read_email, User, hide_email: false # view helper - can :read, Activity - can :read, Team - can :read, Project - can :read, :feed_entry # check: + can :read_email, User, hide_email: false # view helper # delete? only used once + can :read, PUBLIC_PAGES # pro forma ; Activity has no authorisation restriction, except for kind: :mailing + return unless user.confirmed? # confirmed user can [:update, :destroy], User, id: user.id can :resend_confirmation_instruction, User, id: user.id - can :read, :mailing if signed_in?(user) + can :index, Mailing can :read, Mailing do |mailing| mailing.recipient? user end - can :create, Project if user.confirmed? + can :create, Project + # can :crud, Team do |team| + # team.new_record? + # end ???? => delete + + # all members in a team + # if user in any team ... end # or just A confirmed user + can :crud, Team do |team| + on_team?(user, team) + end + + # what is restricted to the current season ? # current_student - can :crud, Conference if user.current_student? + if user.current_student? # TODO is this the best check? + can :create, Conference + end # supervisor - can :read, :users_info if user.supervisor? - can :read_email, User do |other_user| - user.confirmed? && (supervises?(other_user, user) || !other_user.hide_email?) + if user.supervisor? + can :read, :users_info + # explanation for this simpler declaration: + # The unconfirmed user ^ above had this declaration: + # `can :read_email, User, hide_email: false` + # is defined for all users: all can read an email address that is not hidden + # Here, the hide_email attribute doesnt matter: a supervisor can read it anyway + # See specs added to check this behaviour + can :read_email, User do |other_user| + supervises?(other_user, user) + end end # project submitter @@ -47,17 +71,18 @@ def initialize(user) # admin if user.admin? can :manage, :all - can :read_email, User if user.admin? # even when user marked email hidden # view helper - # add cannot's only; after this line + # can :read_email, User # view helper # redundant; admin can manage all + # used only once -> delete? + # MEMO add cannot's only; and only after this line cannot :create, User # this only happens through GitHub end ################# OLD FILE, # = moved to or rewritten above ############ # NOT everything moved yet # - can :crud, Team do |team| - user.admin? || signed_in?(user) && team.new_record? || on_team?(user, team) - end + # can :crud, Team do |team| + # user.admin? || signed_in?(user) && team.new_record? || on_team?(user, team) + # end can :update_conference_preferences, Team do |team| team.accepted? && team.students.include?(user) @@ -97,6 +122,7 @@ def initialize(user) # applications can :create, :application_draft if user.student? && user.application_drafts.in_current_season.none? + end # initializer def signed_in?(user) diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index fd6e615f6..256720507 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -30,5 +30,9 @@ trait :in_current_season do season { Season.current } end + + trait :in_last_season do + season { Season.find_or_create_by name: (Date.today.year - 1) } + end end end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 1df829d9a..5c1f8e108 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -1,58 +1,102 @@ require 'rails_helper' require 'cancan/matchers' +# It is recommended to run this file with +# $ rspec spec/models/ability_spec.rb -fd +# for a nice output of the specs running inside +# the shared examples [mdv] + RSpec.describe Ability, type: :model do subject(:ability) { Ability.new(user) } let(:user){ nil } let(:other_user) { create(:user) } - public_pages = [Activity, Team, Project ].freeze - user_pages = [User] - restricted_pages = [Application, ApplicationDraft] # todo - - # NOTE FOR REVIEW : WIP, added for illustration - - # The specs follow the sequence from the Ability class - # I added a few specs, especially for User class for different users, - # as an example of the new set up - # The shared examples will move to spec/support. - # - - describe "Guest User" do it_behaves_like "can read public pages" + it_behaves_like "can not modify things on public pages" it_behaves_like "can not create new user" + it_behaves_like "can not comment" it_behaves_like "has no access to other user's accounts" - it "can not modify anything" do - public_pages.each do |page| - expect(subject).not_to be_able_to([:create, :update, :destroy], page) - end - end + it_behaves_like "can not read role restricted or owner restricted pages" # now: ApplicationDraft it "can not even modify their own account" do - true # pro memorie - end - it "can not read restricted pages" do - expect(subject).not_to be_able_to(:read, ApplicationDraft) + true end end describe "Logged in user, not confirmed" do - let(:user) { create(:user, :unconfirmed) } + let(:user) { build_stubbed(:user, :unconfirmed) } - it_behaves_like "can not create new user" - it_behaves_like "has no access to other user's accounts" - it_behaves_like "can modify own account" - it_behaves_like "can read public pages" - it_behaves_like "can do more stuff when logged in" + it_behaves_like 'same as guest user' + # plus ... + it_behaves_like "can modify own account" # User should always be able to update email address and resend mail end describe "Confirmed user" do - let(:user) { create(:user) } # default: confirmed - it_behaves_like "can not create new user" - it_behaves_like "has no access to other user's accounts" - it_behaves_like "can modify own account" - it_behaves_like "can read public pages" - it_behaves_like "can do more stuff when logged in" + let(:user) { build_stubbed(:user) } # default: confirmed + + it_behaves_like "same as logged in user" + it_behaves_like "can create a Project" + it_behaves_like "can see mailings list too" + it_behaves_like "can read mailings sent to them" + # it_behaves_like "can comment now" # not implemented; needs work + + describe "Student" do + before do + # TODO Questions: is this stub correct? Is this testing the ability only? + # See also question in Ability + allow(user).to receive(:current_student?).and_return(true) + end + + it_behaves_like "same public features as confirmed user" + it "can create a conference" do + expect(subject).to be_able_to(:create, Conference) + end + end + + describe "Supervisor" do + let(:other_user) { create(:user) } + let(:other_team_user) { build_stubbed(:user) } + + before do + allow(user).to receive(:supervisor?).and_return(true) + allow(ability).to receive(:supervises?) { false } # Rspec complained: "stub default value first" + allow(ability).to receive(:supervises?).with(other_user, user).and_return(true) + allow(other_user).to receive(:hide_email).and_return(true) + allow(other_team_user).to receive(:hide_email).and_return(true) + end + + it_behaves_like "same public features as confirmed user" + # plus... + it { expect(subject).to be_able_to(:read, :users_info, other_user) } + # Check this ^ means they can read users_info of ALL users. ?? Not only their own team members + it { expect(subject).to be_able_to(:read_email, other_user) } + it { expect(subject).not_to be_able_to(:read_email, other_team_user )} + end + + describe "Team member" do + before { create :student_role, team: student_team, user: user } + let(:student_team) { create(:team, :in_current_season) } + let(:other_team) { build_stubbed(:team, :in_current_season) } + let(:future_team) { build(:team, season: Season.succ )} + + + it_behaves_like "same public features as confirmed user" + it { expect(subject).to be_able_to :crud, student_team } + it { expect(subject).not_to be_able_to :update, other_team} # only own team + it { expect(subject).not_to be_able_to :create, other_team } # only one team + xit { expect(subject).to be_able_to :create, future_team } #fails; should pass right? + + end + + describe "Project Submitter" do + let(:old_project) { build_stubbed(:project, submitter: user)} + + it_behaves_like "same public features as confirmed user" + it { expect(subject).to be_able_to(:crud, Project.new(submitter: user)) } + it { expect(subject).to be_able_to(:use_as_template, old_project )} + + end + # TODO # it_behaves_like "team member" @@ -60,8 +104,16 @@ end describe "Admin" do + let(:user) { create(:user) } + let(:other_user) { build_stubbed(:user, hide_email: true) } + + before { allow(user).to receive(:admin?) { true } } + it_behaves_like "can not create new user" # it_behaves_like "has access to almost everything else" + # to name the most exclusive and sensitive + it { expect(subject).to be_able_to(:crud, Team ) } + it { expect(subject).to be_able_to(:read_email, other_user) } end @@ -119,6 +171,7 @@ context "when user's supervisor in current season (confirmed)" do before do allow(user).to receive(:admin?).and_return(false) + allow(user).to receive(:supervisor?).and_return(true) allow(ability).to receive(:supervises?).with(other_user, user).and_return(true) allow(user).to receive(:confirmed?).and_return(true) end @@ -299,8 +352,8 @@ end describe 'access to mailings' do - let!(:mailing) { Mailing.new } - let!(:user) { create(:student) } + let!(:mailing) { create(:mailing) } + let!(:user) { create(:student, confirmed_at: Date.yesterday) } context 'when user is a recipient' do it 'allows to read' do @@ -311,6 +364,7 @@ context 'when user has nothing to do with the mailing' do it 'will not allow to read' do + mailing.to = %w(coaches) expect(subject).not_to be_able_to :read, mailing end end @@ -344,6 +398,7 @@ let!(:user) { create :student } it 'allows crud on new team' do + pending "fails with new abilities, new tests pass" expect(subject).to be_able_to :crud, Team.new end @@ -397,16 +452,18 @@ context 'permitting activities' do context 'for feed_entries' do it 'allows anyone to read' do - expect(Ability.new(nil)).to be_able_to :read, :feed_entry + expect(Ability.new(nil)).to be_able_to :read, Activity, kind: :feed_entry end end + + context 'for mailings' do it 'does not allow anonymous user to read' do - expect(Ability.new(nil)).not_to be_able_to :read, :mailing + expect(Ability.new(nil)).not_to be_able_to :index, :mailing end it 'allows signed in user to read' do - expect(subject).to be_able_to :read, :mailing + expect(subject).to be_able_to :index, Mailing end end end @@ -489,10 +546,14 @@ context 'Crud Conferences' do let!(:user) { create(:user) } + it { puts user.teams.any? } + it { puts user.teams.in_current_season.any? } it 'permit crud conference when user is a current student' do create :student_role, user: user - expect(ability).to be_able_to(:crud, Conference.new) + # expect(ability).to be_able_to(:crud, Conference.new) + # weird that it passed + end it 'permit crud conference when user is an organizer' do diff --git a/spec/support/shared_examples/abilities.rb b/spec/support/shared_examples/abilities.rb index 9d2def7f5..eb8ce9d2d 100644 --- a/spec/support/shared_examples/abilities.rb +++ b/spec/support/shared_examples/abilities.rb @@ -1,43 +1,98 @@ -# TODO find a place to put the arrays once, not here and in the spec -public_pages = [Activity, Team, Project ].freeze -user_pages = [User] -restricted_pages = [Application, ApplicationDraft] # todo +# collections of shared examples + +# logged in, unconfirmed: +shared_examples "same as guest user" do + it_behaves_like "can read public pages" + it_behaves_like "can not modify things on public pages" + it_behaves_like "can not create new user" + it_behaves_like "can not comment" #(= logged in pages ) + it_behaves_like "has no access to other user's accounts" + it_behaves_like "can not read role restricted or owner restricted pages" # now: ApplicationDraft +end + +# logged in and confirmed +shared_examples "same as logged in user" do + it_behaves_like "can read public pages" + it_behaves_like "can not create new user" + it_behaves_like "can not comment" #(= logged in pages ) + it_behaves_like "has no access to other user's accounts" + it_behaves_like "can not read role restricted or owner restricted pages" # now: ApplicationDraft + it_behaves_like "can modify own account" +end + +# different roles +shared_examples "same public features as confirmed user" do + it_behaves_like "can read public pages" + it_behaves_like "can not create new user" + it_behaves_like "can modify own account" +end + + +# details shared_examples 'can read public pages' do - it "has access to public pages" do - public_pages.each do |page| - expect(subject).to be_able_to(:read, page) - end + Ability::PUBLIC_PAGES.each do |page| + it { expect(subject).to be_able_to(:read, page) } end - it {expect(subject).to be_able_to(:read, User) } + it { expect(subject).to be_able_to(:index, Activity, kind: :feed_entry) } #TODO delete + it { expect(subject).not_to be_able_to(:index, Activity.with_kind(:mailing)) } #TODO extract + it { expect(subject).to be_able_to(:read, User) } end -shared_examples "has access to restricted pages" do - restricted_pages.each do |page| - expect(subject).to be_able_to(:read, page) +shared_examples "can not modify things on public pages" do + Ability::PUBLIC_PAGES.each do |page| + it { expect(subject).not_to be_able_to([:create, :update, :destroy], page) } end end +shared_examples "can create a Project" do + it { expect(subject).to be_able_to(:create, Project) } +end + +shared_examples 'can not create new user' do + it { expect(subject).not_to be_able_to(:create, User.new) } +end + +shared_examples "can not comment" do + Ability::LOGGED_IN_PAGES.each do |page| + it { expect(subject).not_to be_able_to([:create, :update, :destroy], page) } + end +end + +shared_examples "can not read role restricted or owner restricted pages" do #ApplicationDraft + it { expect(subject).not_to be_able_to(:read, ApplicationDraft) } #todo abstraction, collect pages first + # restricted_pages.each do |page| + # expect(subject).to be_able_to(:read, page) + # end +end + # Shared examples for User shared_examples "has no access to other user's accounts" do # pro memorie: outside their team + let(:other_user) { create(:user)} it { expect(subject).not_to be_able_to([:update, :destroy], other_user) } end shared_examples "can modify own account" do + let(:user) {create(:user) } + it { expect(subject).to be_able_to([:update, :destroy], user) } + it { expect(subject).to be_able_to(:resend_confirmation_instruction, User, id: user.id) } end -shared_examples 'can not create new user' do - it { expect(subject).not_to be_able_to(:create, User.new) } +shared_examples "can comment now" do + it { expect(subject).to be_able_to(:crud, Comment) } # TODO returns false positive; needs work end -# Shared examples for logged in users -shared_examples "can do more stuff when logged in" do - it 'can add comments' do # - pending "todo" - raise "NotImplementedFakeError" - end - # todo and more + +# This is role based? to = array of team members etc +shared_examples "can see mailings list too" do + it { expect(subject).to be_able_to(:index, Mailing) } +end + + +shared_examples "can read mailings sent to them" do + let(:user) { create(:user) } + it { expect(subject).to be_able_to(:read, Mailing, recipient: user )} end # Todo add shared_examples for other roles etc From bfaeb99eafd036e43e200da8fda1431e37bc3445 Mon Sep 17 00:00:00 2001 From: F3PiX Date: Thu, 17 May 2018 15:09:39 +0200 Subject: [PATCH 05/20] WIP All things processed and annotated --- app/models/ability.rb | 72 +- spec/models/ability_spec.rb | 762 +++++++++++----------- spec/support/shared_examples/abilities.rb | 53 +- 3 files changed, 446 insertions(+), 441 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index fe1d7e565..ce12b83d3 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -3,67 +3,69 @@ class Ability include CanCan::Ability - USER_PAGES = [User, Team] #todo - PUBLIC_PAGES = [Activity, Team, Project, Conference].freeze - LOGGED_IN_PAGES = [Comment] # now: create comments #todo - APPLICATION_PAGES = [Application, ApplicationDraft] # todo - def initialize(user) user ||= User.new alias_action :create, :read, :update, :destroy, to: :crud + # guest user - can :read, [User, Team, Project, Activity] + can :read, Activity # pro forma ; Activity has no authorisation restriction, except for kind: :mailing + can :read, [User, Team, Project, Conference] + + return unless signed_in?(user) # unconfirmed, logged in user - can :read, USER_PAGES can :update, User, id: user.id can :resend_confirmation_instruction, User, id: user.id can :read_email, User, hide_email: false # view helper # delete? only used once - can :read, PUBLIC_PAGES # pro forma ; Activity has no authorisation restriction, except for kind: :mailing - return unless user.confirmed? + return unless user.confirmed? && signed_in?(user) + # confirmed user can [:update, :destroy], User, id: user.id can :resend_confirmation_instruction, User, id: user.id + can :create, Project + can [:join, :create], Team + # the validation is tested in spec/models/team_spec.rb:33 + # Should this be in ability at all? + # can :crud, Team do |team| + # team.new_record? + # end + # => delete can :index, Mailing can :read, Mailing do |mailing| mailing.recipient? user end - can :create, Project - # can :crud, Team do |team| - # team.new_record? - # end ???? => delete - # all members in a team - # if user in any team ... end # or just A confirmed user - can :crud, Team do |team| + # Members in a team + can [:update, :destroy], Team do |team| on_team?(user, team) end - # what is restricted to the current season ? + # Add / split restrictions for current season? # current_student - if user.current_student? # TODO is this the best check? + if user.current_student? # TODO is this the best check? + can :create, Team if user.teams.none? can :create, Conference end # supervisor if user.supervisor? - can :read, :users_info - # explanation for this simpler declaration: - # The unconfirmed user ^ above had this declaration: - # `can :read_email, User, hide_email: false` - # is defined for all users: all can read an email address that is not hidden - # Here, the hide_email attribute doesnt matter: a supervisor can read it anyway - # See specs added to check this behaviour - can :read_email, User do |other_user| - supervises?(other_user, user) - end + can :read, :users_info + # explanation for this simpler declaration: + # The unconfirmed user ^ above had this declaration: + # `can :read_email, User, hide_email: false` + # is defined for all users: all can read an email address that is not hidden + # Here, the hide_email attribute doesnt matter: a supervisor can read it anyway + # See specs added to check this behaviour + can :read_email, User do |other_user| + supervises?(other_user, user) + end end # project submitter - can :crud, Project, submitter_id: user.id if user.confirmed? + can [:update, :destroy], Project, submitter_id: user.id can :use_as_template, Project do |project| user == project.submitter && !project.season&.current? end @@ -71,9 +73,7 @@ def initialize(user) # admin if user.admin? can :manage, :all - # can :read_email, User # view helper # redundant; admin can manage all - # used only once -> delete? - # MEMO add cannot's only; and only after this line + # MEMO add "cannot's" only; and only after this line cannot :create, User # this only happens through GitHub end @@ -96,10 +96,10 @@ def initialize(user) team.students.include?(user) end - cannot :create, Team do |team| - on_team_for_season?(user, team.season) || !user.confirmed? - end - + # cannot :create, Team do |team| + # on_team_for_season?(user, team.season) || !user.confirmed? + # end + # todo join helpdesk team can :join, Team do |team| team.helpdesk_team? and signed_in?(user) and user.confirmed? and not on_team?(user, team) end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 5c1f8e108..5970d47fe 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -1,10 +1,9 @@ require 'rails_helper' require 'cancan/matchers' -# It is recommended to run this file with -# $ rspec spec/models/ability_spec.rb -fd -# for a nice output of the specs running inside -# the shared examples [mdv] +# Run this file with +# $ rspec spec/models/ability_spec.rb -fd +# to see the output of specs running inside the shared examples [mdv] RSpec.describe Ability, type: :model do subject(:ability) { Ability.new(user) } @@ -12,108 +11,118 @@ let(:other_user) { create(:user) } describe "Guest User" do - it_behaves_like "can read public pages" + it_behaves_like "can view public pages" it_behaves_like "can not modify things on public pages" it_behaves_like "can not create new user" it_behaves_like "can not comment" it_behaves_like "has no access to other user's accounts" it_behaves_like "can not read role restricted or owner restricted pages" # now: ApplicationDraft - it "can not even modify their own account" do + it "does not have an account" do true end end describe "Logged in user, not confirmed" do - let(:user) { build_stubbed(:user, :unconfirmed) } + let(:user) { build_stubbed(:user, :unconfirmed) } it_behaves_like 'same as guest user' - # plus ... it_behaves_like "can modify own account" # User should always be able to update email address and resend mail end describe "Confirmed user" do - let(:user) { build_stubbed(:user) } # default: confirmed + let!(:user) { create(:user) } it_behaves_like "same as logged in user" it_behaves_like "can create a Project" it_behaves_like "can see mailings list too" it_behaves_like "can read mailings sent to them" - # it_behaves_like "can comment now" # not implemented; needs work + it_behaves_like "can comment now" # not implemented; false positives; needs work + it { expect(subject).to be_able_to([:join, :create], Team) } - describe "Student" do - before do - # TODO Questions: is this stub correct? Is this testing the ability only? - # See also question in Ability - allow(user).to receive(:current_student?).and_return(true) - end + describe "Team" do + describe 'All members' do + let(:current_team) { create(:team, :in_current_season) } + let(:other_team) { build_stubbed(:team, :in_current_season) } + let(:future_team) { build(:team, season: Season.succ )} - it_behaves_like "same public features as confirmed user" - it "can create a conference" do - expect(subject).to be_able_to(:create, Conference) + # must be true for all roles but student; can that be stubbed or something? + before { create :coach_role, team: current_team, user: user } + + it_behaves_like "same public features as confirmed user" + it { expect(subject).to be_able_to [:update, :destroy], current_team } + it { expect(subject).not_to be_able_to [:update, :destroy], other_team} end - end - describe "Supervisor" do - let(:other_user) { create(:user) } - let(:other_team_user) { build_stubbed(:user) } + describe "Student" do + let(:student_team) { build(:team, :in_current_season) } + let(:second_team) { build(:team, :in_current_season) } + before do + # create :student_role, team: student_team, user: user + allow(user).to receive(:current_student?).and_return(true) + end + # TODO Questions: is this ^ stub correct to test the ability only? - before do - allow(user).to receive(:supervisor?).and_return(true) - allow(ability).to receive(:supervises?) { false } # Rspec complained: "stub default value first" - allow(ability).to receive(:supervises?).with(other_user, user).and_return(true) - allow(other_user).to receive(:hide_email).and_return(true) - allow(other_team_user).to receive(:hide_email).and_return(true) - end + it_behaves_like "same public features as confirmed user" + it { expect(subject).to be_able_to(:create, Conference) } - it_behaves_like "same public features as confirmed user" - # plus... - it { expect(subject).to be_able_to(:read, :users_info, other_user) } - # Check this ^ means they can read users_info of ALL users. ?? Not only their own team members - it { expect(subject).to be_able_to(:read_email, other_user) } - it { expect(subject).not_to be_able_to(:read_email, other_team_user )} - end + it "can create their first team" do + expect(subject).to be_able_to(:create, student_team) + end + it 'cannot create a second team in current_season' do + pending 'fails; it is tested in team_spec.rb:33, and that passes' + expect(subject).not_to be_able_to(:create, second_team) + end - describe "Team member" do - before { create :student_role, team: student_team, user: user } - let(:student_team) { create(:team, :in_current_season) } - let(:other_team) { build_stubbed(:team, :in_current_season) } - let(:future_team) { build(:team, season: Season.succ )} + let(:future_team) { build(:team, season: Season.succ ) } + it { expect(subject).to be_able_to :create, future_team } + end + describe "Supervisor" do + let(:other_user) { create(:user) } + let(:other_team_user) { build_stubbed(:user) } - it_behaves_like "same public features as confirmed user" - it { expect(subject).to be_able_to :crud, student_team } - it { expect(subject).not_to be_able_to :update, other_team} # only own team - it { expect(subject).not_to be_able_to :create, other_team } # only one team - xit { expect(subject).to be_able_to :create, future_team } #fails; should pass right? + before do + allow(user).to receive(:supervisor?).and_return(true) + allow(ability).to receive(:supervises?) { false } # Rspec complained: "stub default value first" + allow(ability).to receive(:supervises?).with(other_user, user).and_return(true) + allow(other_user).to receive(:hide_email).and_return(true) + allow(other_team_user).to receive(:hide_email).and_return(true) + end + it_behaves_like "same public features as confirmed user" + it { expect(subject).to be_able_to(:read, :users_info, other_user) } + it { expect(subject).to be_able_to(:read, :users_info, other_team_user) } # but should they? + it { expect(subject).to be_able_to(:read_email, other_user) } + it { expect(subject).not_to be_able_to(:read_email, other_team_user )} + end end describe "Project Submitter" do - let(:old_project) { build_stubbed(:project, submitter: user)} + let(:old_project) { build_stubbed(:project, submitter: user) } + let(:other_project) { build_stubbed(:project, submitter: other_user) } + let(:same_season_project) { build :project, :in_current_season, submitter: user } it_behaves_like "same public features as confirmed user" - it { expect(subject).to be_able_to(:crud, Project.new(submitter: user)) } - it { expect(subject).to be_able_to(:use_as_template, old_project )} - + it { expect(subject).to be_able_to([:update, :destroy], Project.new(submitter: user)) } + it { expect(subject).to be_able_to(:use_as_template, old_project) } + it { expect(subject).not_to be_able_to(:use_as_template, other_project) } + it { expect(subject).not_to be_able_to :use_as_template, same_season_project } end - - - # TODO - # it_behaves_like "team member" - # etc end describe "Admin" do let(:user) { create(:user) } - let(:other_user) { build_stubbed(:user, hide_email: true) } + let(:other_user) { build_stubbed(:user, hide_email: true) } before { allow(user).to receive(:admin?) { true } } it_behaves_like "can not create new user" - # it_behaves_like "has access to almost everything else" - # to name the most exclusive and sensitive - it { expect(subject).to be_able_to(:crud, Team ) } + # it "has access to almost everything else" + # To name the most exclusive and sensitive + it { expect(subject).to be_able_to(:crud, Team) } + it { expect(subject).to be_able_to([:read, :update, :destroy], User) } it { expect(subject).to be_able_to(:read_email, other_user) } + it { expect(subject).to be_able_to(:read, :users_info, other_user) } end @@ -121,149 +130,149 @@ ############# OLD SPECS ################ context 'ability' do - context 'when a user is connected' do - let!(:user) { create(:user) } - let!(:team) { create(:team) } - - describe 'she/he is allowed to do everything on her/his account' do - it { expect(ability).to be_able_to(:show, user) } - it { expect(ability).not_to be_able_to(:create, User.new) } # this only happens through GitHub - - it { expect(ability).to be_able_to(:resend_confirmation_instruction, user) } - end - - context 'when a user is admin' do - let(:organizer_role) { create(:organizer_role, user: user) } - before { allow(organizer_role).to receive(:admin?).and_return(true) } - - it "should be able to CRUD on anyone's account" do - expect(subject).to be_able_to(:crud, organizer_role) - end - - describe 'she/he is not allowed to CRUD on someone else account' do - let(:other_user) { create(:user) } - # But an admin should! show and crud - xit { expect(ability).not_to be_able_to(:show, other_user) } - end - end - - - - describe 'who is allowed to see email address in user profile' do - - # email address is hidden: admin, user's supervisor in current season (confirmed) - # email address is not hidden: admin, confirmed user, user him/herself - - let(:other_user) { create(:user) } - - context 'when email address is hidden' do - context 'when an admin' do - before do - allow(user).to receive(:admin?).and_return(true) - allow(ability).to receive(:supervises?).with(other_user, user).and_return(false) - end - it 'allows to see hidden email address' do - other_user.hide_email = true - expect(ability).to be_able_to(:read_email, other_user) - end - end - - context "when user's supervisor in current season (confirmed)" do - before do - allow(user).to receive(:admin?).and_return(false) - allow(user).to receive(:supervisor?).and_return(true) - allow(ability).to receive(:supervises?).with(other_user, user).and_return(true) - allow(user).to receive(:confirmed?).and_return(true) - end - it 'allows to see hidden email address' do - other_user.hide_email = true - expect(ability).to be_able_to(:read_email, other_user) - end - end - end - - context 'when email address is not hidden' do - context 'when an admin' do - before do - allow(user).to receive(:admin?).and_return(true) - allow(user).to receive(:confirmed?).and_return(false) - end - it 'allows to see not hidden email address' do - other_user.hide_email = false - expect(ability).to be_able_to(:read_email, other_user) - end - end - - context 'when a confirmed user' do - before do - allow(user).to receive(:admin?).and_return(false) - allow(user).to receive(:confirmed?).and_return(true) - end - it 'allows to see not hidden email address' do - other_user.hide_email = false - expect(ability).to be_able_to(:read_email, other_user) - end - end - - context 'when the user him/herself' do - it 'allows to see not hidden email address' do - user.hide_email = false - expect(ability).to be_able_to(:read_email, user) - end - end - end - end - - describe 'who is disallowed to see email address in user profile' do - - # email address is hidden: not admin, not user's supervisor in current season - # email address is not hidden: not admin, not confirmed user, not user him/herself - - let(:other_user) { create(:user) } - - context 'when email address is hidden' do - context "when not an admin or user's supervisor in current season" do - before do - allow(user).to receive(:admin?).and_return(false) - allow(ability).to receive(:supervises?).with(other_user, user).and_return(false) - end - it 'disallows to see hidden email address' do - other_user.hide_email = true - expect(ability).not_to be_able_to(:read_email, other_user) - end - end - end - - context 'when email address is not hidden' do - context 'when not an admin or confirmed user or user him/herself' do - before do - allow(user).to receive(:admin?).and_return(false) - allow(user).to receive(:confirmed?).and_return(false) - end - # NOTE / TODO is this testing "can? read_email" properly? - xit 'disallows to see not hidden email address' do - other_user.hide_email = false - expect(ability).not_to be_able_to(:read_email, other_user) - end - end - end - - end - - - context 'resend_confirmation_instruction' do - let(:other_user) { create(:user) } - - describe 'a user can only resend her/his own confirmation' do - it { expect(ability).to be_able_to(:resend_confirmation_instruction, user) } - it { expect(ability).not_to be_able_to(:resend_confirmation_instruction, other_user) } - end - - describe 'a admin can resend all confirmation tokens' do - let!(:organizer_role) { create(:organizer_role, user: user) } - it { expect(ability).to be_able_to(:resend_confirmation_instruction, other_user) } - end - end + # context 'when a user is connected' do + # let!(:user) { create(:user) } + # let!(:team) { create(:team) } + # + # describe 'she/he is allowed to do everything on her/his account' do + # it { expect(ability).to be_able_to(:show, user) } + # it { expect(ability).not_to be_able_to(:create, User.new) } # this only happens through GitHub + # + # it { expect(ability).to be_able_to(:resend_confirmation_instruction, user) } + # end + # + # context 'when a user is admin' do + # let(:organizer_role) { create(:organizer_role, user: user) } + # before { allow(organizer_role).to receive(:admin?).and_return(true) } + # + # it "should be able to CRUD on anyone's account" do + # expect(subject).to be_able_to(:crud, organizer_role) + # end + # + # describe 'she/he is not allowed to CRUD on someone else account' do + # let(:other_user) { create(:user) } + # # But an admin should! show and crud + # xit { expect(ability).not_to be_able_to(:show, other_user) } + # end + # end + + + + # describe 'who is allowed to see email address in user profile' do + # + # # email address is hidden: admin, user's supervisor in current season (confirmed) + # # email address is not hidden: admin, confirmed user, user him/herself + # + # let(:other_user) { create(:user) } + # + # context 'when email address is hidden' do + # context 'when an admin' do + # before do + # allow(user).to receive(:admin?).and_return(true) + # allow(ability).to receive(:supervises?).with(other_user, user).and_return(false) + # end + # it 'allows to see hidden email address' do + # other_user.hide_email = true + # expect(ability).to be_able_to(:read_email, other_user) + # end + # end + # + # context "when user's supervisor in current season (confirmed)" do + # before do + # allow(user).to receive(:admin?).and_return(false) + # allow(user).to receive(:supervisor?).and_return(true) + # allow(ability).to receive(:supervises?).with(other_user, user).and_return(true) + # allow(user).to receive(:confirmed?).and_return(true) + # end + # it 'allows to see hidden email address' do + # other_user.hide_email = true + # expect(ability).to be_able_to(:read_email, other_user) + # end + # end + # end + + # context 'when email address is not hidden' do + # context 'when an admin' do + # before do + # allow(user).to receive(:admin?).and_return(true) + # allow(user).to receive(:confirmed?).and_return(false) + # end + # it 'allows to see not hidden email address' do + # other_user.hide_email = false + # expect(ability).to be_able_to(:read_email, other_user) + # end + # end + # + # context 'when a confirmed user' do + # before do + # allow(user).to receive(:admin?).and_return(false) + # allow(user).to receive(:confirmed?).and_return(true) + # end + # it 'allows to see not hidden email address' do + # other_user.hide_email = false + # expect(ability).to be_able_to(:read_email, other_user) + # end + # end + # + # context 'when the user him/herself' do + # it 'allows to see not hidden email address' do + # user.hide_email = false + # expect(ability).to be_able_to(:read_email, user) + # end + # end + # end + # end + + # describe 'who is disallowed to see email address in user profile' do + # + # # email address is hidden: not admin, not user's supervisor in current season + # # email address is not hidden: not admin, not confirmed user, not user him/herself + # + # let(:other_user) { create(:user) } + # + # context 'when email address is hidden' do + # context "when not an admin or user's supervisor in current season" do + # before do + # allow(user).to receive(:admin?).and_return(false) + # allow(ability).to receive(:supervises?).with(other_user, user).and_return(false) + # end + # it 'disallows to see hidden email address' do + # other_user.hide_email = true + # expect(ability).not_to be_able_to(:read_email, other_user) + # end + # end + # end + # + # context 'when email address is not hidden' do + # context 'when not an admin or confirmed user or user him/herself' do + # before do + # allow(user).to receive(:admin?).and_return(false) + # allow(user).to receive(:confirmed?).and_return(false) + # end + # # NOTE / TODO is this testing "can? read_email" properly? + # xit 'disallows to see not hidden email address' do + # other_user.hide_email = false + # expect(ability).not_to be_able_to(:read_email, other_user) + # end + # end + # end + # + # end + + + # context 'resend_confirmation_instruction' do + # let(:other_user) { create(:user) } + # + # describe 'a user can only resend her/his own confirmation' do + # it { expect(ability).to be_able_to(:resend_confirmation_instruction, user) } + # it { expect(ability).not_to be_able_to(:resend_confirmation_instruction, other_user) } + # end + # + # describe 'a admin can resend all confirmation tokens' do + # let!(:organizer_role) { create(:organizer_role, user: user) } + # it { expect(ability).to be_able_to(:resend_confirmation_instruction, other_user) } + # end + # end describe "team's students or admin should be able to mark preferences to a conference" do context 'when user is a student from a team and try to update conference preferences' do @@ -286,13 +295,14 @@ context 'when different users' do let!(:other_user) { create(:user)} let!(:conference_preference) { create(:conference_preference, team: team)} - it { expect(ability).not_to be_able_to(:crud, other_user) } + + xit { expect(ability).not_to be_able_to(:crud, other_user) } end end describe "just orga members, team's supervisor and team's students should be able to see offered conference for a team" do - let(:user) { build(:student)} + let(:user) { create(:student, confirmed_at: Date.yesterday)} context 'when the user is an student of another team' do it { expect(ability).not_to be_able_to(:see_offered_conferences, Team.new) } @@ -321,103 +331,103 @@ end end - describe 'to read user info' do - context 'if not an admin or supervisor' do - before do - allow(user).to receive(:admin?).and_return(false) - allow(user).to receive(:supervisor?).and_return(false) - end - - it { expect(ability).not_to be_able_to(:read, :users_info) } - end - - context 'if an admin' do - before do - allow(user).to receive(:admin?).and_return(true) - allow(user).to receive(:supervisor?).and_return(false) - end - - it { expect(ability).to be_able_to(:read, :users_info) } - end - - context 'if a supervisor' do - before do - allow(user).to receive(:admin?).and_return(false) - allow(user).to receive(:supervisor?).and_return(true) - end - - it { expect(ability).to be_able_to(:read, :users_info) } - end - - end - - describe 'access to mailings' do - let!(:mailing) { create(:mailing) } - let!(:user) { create(:student, confirmed_at: Date.yesterday) } - - context 'when user is a recipient' do - it 'allows to read' do - mailing.to = %w(students) - expect(subject).to be_able_to :read, mailing - end - end - - context 'when user has nothing to do with the mailing' do - it 'will not allow to read' do - mailing.to = %w(coaches) - expect(subject).not_to be_able_to :read, mailing - end - end - end + # describe 'to read user info' do + # context 'if not an admin or supervisor' do + # before do + # allow(user).to receive(:admin?).and_return(false) + # allow(user).to receive(:supervisor?).and_return(false) + # end + # + # it { expect(ability).not_to be_able_to(:read, :users_info) } + # end + # + # context 'if an admin' do + # before do + # allow(user).to receive(:admin?).and_return(true) + # allow(user).to receive(:supervisor?).and_return(false) + # end + # + # it { expect(ability).to be_able_to(:read, :users_info) } + # end + # + # context 'if a supervisor' do + # before do + # allow(user).to receive(:admin?).and_return(false) + # allow(user).to receive(:supervisor?).and_return(true) + # end + # + # it { expect(ability).to be_able_to(:read, :users_info) } + # end + # + # end + + # describe 'access to mailings' do + # let!(:mailing) { create(:mailing) } + # let!(:user) { create(:student, confirmed_at: Date.yesterday) } + # + # context 'when user is a recipient' do + # it 'allows to read' do + # mailing.to = %w(students) + # expect(subject).to be_able_to :read, mailing + # end + # end + # + # context 'when user has nothing to do with the mailing' do + # it 'will not allow to read' do + # mailing.to = %w(coaches) + # expect(subject).not_to be_able_to :read, mailing + # end + # end + # end describe 'operations on teams' do let(:team) { create :team } - context 'when user admin' do - let(:user) { create :organizer } - - it 'allows crud' do - expect(subject).to be_able_to :crud, team - end - end - - context 'when not confirmed' do - let!(:user) { create :student, confirmed_at: nil } - let(:team) { create(:team) } - - it 'does not allow to create teams' do - expect(subject).not_to be_able_to :create, Team.new - end - - it 'does not allow to join teams' do - expect(subject).not_to be_able_to :join, team - end - end + # context 'when user admin' do + # let(:user) { create :organizer } + # + # it 'allows crud' do + # expect(subject).to be_able_to :crud, team + # end + # end + + # context 'when not confirmed' do + # let!(:user) { create :student, confirmed_at: nil } + # let(:team) { create(:team) } + # + # it 'does not allow to create teams' do + # expect(subject).not_to be_able_to :create, Team.new + # end + # + # it 'does not allow to join teams' do + # expect(subject).not_to be_able_to :join, team + # end + # end context 'when user student' do let!(:user) { create :student } - it 'allows crud on new team' do - pending "fails with new abilities, new tests pass" - expect(subject).to be_able_to :crud, Team.new - end - - it 'allows crud on own team' do - create :student_role, team: team, user: user - expect(subject).to be_able_to :crud, team - end - - it 'does not allow crud on other team' do - expect(subject).not_to be_able_to :crud, team - end + # it 'allows crud on new team' do + # pending "fails with new abilities, new tests pass" + # expect(subject).to be_able_to :crud, Team.new + # end + # + # it 'allows crud on own team' do + # create :student_role, team: team, user: user + # expect(subject).to be_able_to :crud, team + # end + # + # it 'does not allow crud on other team' do + # expect(subject).not_to be_able_to :crud, team + # end context 'when in team for season' do before { create :student_role, team: student_team, user: user } let(:student_team) { create(:team, :in_current_season) } - it 'allows crud on own team' do - expect(subject).to be_able_to :crud, student_team - end + # it 'allows crud on own team' do + # expect(subject).to be_able_to :crud, student_team + # end it 'allows update conferences on own team' do expect(subject).to be_able_to :update_conference_preferences, student_team @@ -427,102 +437,102 @@ expect(subject).not_to be_able_to :update_conference_preferences, Team.new end - it 'allows to create team for different season' do - expect(subject).to be_able_to :create, Team.new - end - - it 'does not allow to create team for same season' do - expect(subject).not_to be_able_to :create, Team.new(season: student_team.season) - end - end - end - context 'when user guest (not persisted)' do - let(:user) { build :user } - - it 'does not allow crud on existing team' do - expect(subject).not_to be_able_to :crud, team - end + # it 'allows to create team for different season' do + # expect(subject).to be_able_to :create, Team.new + # end - it 'does not allow to create team' do - expect(subject).not_to be_able_to :create, Team.new + # it 'does not allow to create team for same season' do + # expect(subject).not_to be_able_to :create, Team.new(season: student_team.season) + # end end end + # context 'when user guest (not persisted)' do + # let(:user) { build :user } + # + # it 'does not allow crud on existing team' do + # expect(subject).not_to be_able_to :crud, team + # end + # + # it 'does not allow to create team' do + # expect(subject).not_to be_able_to :create, Team.new + # end + # end end context 'permitting activities' do - context 'for feed_entries' do - it 'allows anyone to read' do - expect(Ability.new(nil)).to be_able_to :read, Activity, kind: :feed_entry - end - end - - - - context 'for mailings' do - it 'does not allow anonymous user to read' do - expect(Ability.new(nil)).not_to be_able_to :index, :mailing - end - it 'allows signed in user to read' do - expect(subject).to be_able_to :index, Mailing - end - end + # context 'for feed_entries' do + # it 'allows anyone to read' do + # expect(Ability.new(nil)).to be_able_to :read, Activity, kind: :feed_entry + # end + # end + + + + # context 'for mailings' do + # it 'does not allow anonymous user to read' do + # expect(Ability.new(nil)).not_to be_able_to :index, :mailing + # end + # it 'allows signed in user to read' do + # expect(subject).to be_able_to :index, Mailing + # end + # end end - end + # end context 'working with projects' do let!(:user) { create(:user) } - context 'crud' do - - it 'can be edited when I am an admin' do - create(:organizer_role, user: user) - expect(subject).to be_able_to :crud, Project.new - end - - it 'can be edited if I am the project submitter' do - expect(subject).to be_able_to :crud, Project.new(submitter: user) - end - - it 'cannot be edited if my account is not confirmed' do - user.confirmed_at = nil - user.save - expect(subject).not_to be_able_to :crud, Project.new(submitter: user) - end - - end - - context 'create' do - it 'can be created if I am confirmed' do - expect(subject).to be_able_to :create, Project.new - end - - it 'cannot be created if I am not confirmed' do - user.confirmed_at = nil - user.save - expect(subject).not_to be_able_to :create, Project.new - end - - end - - context 'when using a project as a template' do - let(:user) { build :user } - - context 'for the original project submitter' do - let(:project) { build :project, submitter: user } - it { is_expected.to be_able_to :use_as_template, project } - - context 'for a project from the same season' do - let(:project) { build :project, :in_current_season, submitter: user } - it { is_expected.not_to be_able_to :use_as_template, project } - end - end - - context 'for a project submitted by someone else' do - let(:project) { build :project } - it { is_expected.not_to be_able_to :use_as_template, project } - end - end + # context 'crud' do + # + # # it 'can be edited when I am an admin' do + # # create(:organizer_role, user: user) + # # expect(subject).to be_able_to :crud, Project.new + # # end + # + # # it 'can be edited if I am the project submitter' do + # # expect(subject).to be_able_to :crud, Project.new(submitter: user) + # # end + # + # # it 'cannot be edited if my account is not confirmed' do + # # user.confirmed_at = nil + # # user.save + # # expect(subject).not_to be_able_to :crud, Project.new(submitter: user) + # # end + # + # end + + # context 'create' do + # # it 'can be created if I am confirmed' do + # # expect(subject).to be_able_to :create, Project.new + # # end + # + # # it 'cannot be created if I am not confirmed' do + # # user.confirmed_at = nil + # # user.save + # # expect(subject).not_to be_able_to :create, Project.new + # # end + # + # end + + # context 'when using a project as a template' do + # let(:user) { build :user } + # + # context 'for the original project submitter' do + # let(:project) { build :project, submitter: user } + # # it { is_expected.to be_able_to :use_as_template, project } + # + # context 'for a project from the same season' do + # let(:project) { build :project, :in_current_season, submitter: user } + # it { is_expected.not_to be_able_to :use_as_template, project } + # end + # end + # + # context 'for a project submitted by someone else' do + # let(:project) { build :project } + # it { is_expected.not_to be_able_to :use_as_template, project } + # end + # end end end @@ -546,14 +556,12 @@ context 'Crud Conferences' do let!(:user) { create(:user) } - it { puts user.teams.any? } - it { puts user.teams.in_current_season.any? } it 'permit crud conference when user is a current student' do create :student_role, user: user + # but they shouldn't be able to crud? Only create. # expect(ability).to be_able_to(:crud, Conference.new) - # weird that it passed - + expect(ability).to be_able_to(:create, Conference.new) end it 'permit crud conference when user is an organizer' do diff --git a/spec/support/shared_examples/abilities.rb b/spec/support/shared_examples/abilities.rb index eb8ce9d2d..ad589c179 100644 --- a/spec/support/shared_examples/abilities.rb +++ b/spec/support/shared_examples/abilities.rb @@ -2,19 +2,19 @@ # logged in, unconfirmed: shared_examples "same as guest user" do - it_behaves_like "can read public pages" + it_behaves_like "can view public pages" it_behaves_like "can not modify things on public pages" it_behaves_like "can not create new user" - it_behaves_like "can not comment" #(= logged in pages ) + it_behaves_like "can not comment" it_behaves_like "has no access to other user's accounts" it_behaves_like "can not read role restricted or owner restricted pages" # now: ApplicationDraft end -# logged in and confirmed +# confirmed shared_examples "same as logged in user" do - it_behaves_like "can read public pages" + it_behaves_like "can view public pages" it_behaves_like "can not create new user" - it_behaves_like "can not comment" #(= logged in pages ) + it_behaves_like "can not comment" it_behaves_like "has no access to other user's accounts" it_behaves_like "can not read role restricted or owner restricted pages" # now: ApplicationDraft it_behaves_like "can modify own account" @@ -22,25 +22,29 @@ # different roles shared_examples "same public features as confirmed user" do - it_behaves_like "can read public pages" + it_behaves_like "can view public pages" it_behaves_like "can not create new user" it_behaves_like "can modify own account" + # todo can creates comments; needs work end -# details +# DETAILS , in order of appearance -shared_examples 'can read public pages' do - Ability::PUBLIC_PAGES.each do |page| +PUBLIC_INDEX_PAGES = [Activity, User, Team, Project, Conference].freeze + +# details for unrestricted +shared_examples 'can view public pages' do + PUBLIC_INDEX_PAGES.each do |page| it { expect(subject).to be_able_to(:read, page) } end - it { expect(subject).to be_able_to(:index, Activity, kind: :feed_entry) } #TODO delete - it { expect(subject).not_to be_able_to(:index, Activity.with_kind(:mailing)) } #TODO extract + it { expect(subject).to be_able_to(:index, Activity, kind: :feed_entry) } # TODO delete + it { expect(subject).not_to be_able_to(:index, Activity.with_kind(:mailing)) } it { expect(subject).to be_able_to(:read, User) } end shared_examples "can not modify things on public pages" do - Ability::PUBLIC_PAGES.each do |page| + PUBLIC_INDEX_PAGES.each do |page| it { expect(subject).not_to be_able_to([:create, :update, :destroy], page) } end end @@ -54,24 +58,19 @@ end shared_examples "can not comment" do - Ability::LOGGED_IN_PAGES.each do |page| - it { expect(subject).not_to be_able_to([:create, :update, :destroy], page) } - end + it { expect(subject).not_to be_able_to([:create, :update, :destroy], Comment) } end -shared_examples "can not read role restricted or owner restricted pages" do #ApplicationDraft - it { expect(subject).not_to be_able_to(:read, ApplicationDraft) } #todo abstraction, collect pages first - # restricted_pages.each do |page| - # expect(subject).to be_able_to(:read, page) - # end -end - -# Shared examples for User shared_examples "has no access to other user's accounts" do # pro memorie: outside their team let(:other_user) { create(:user)} it { expect(subject).not_to be_able_to([:update, :destroy], other_user) } end +shared_examples "can not read role restricted or owner restricted pages" do + it { expect(subject).not_to be_able_to(:read, Application, ApplicationDraft) } # todo, collect all pages +end + +# details for logged in shared_examples "can modify own account" do let(:user) {create(:user) } @@ -79,21 +78,19 @@ it { expect(subject).to be_able_to(:resend_confirmation_instruction, User, id: user.id) } end +# details for confirmed + shared_examples "can comment now" do - it { expect(subject).to be_able_to(:crud, Comment) } # TODO returns false positive; needs work + xit { expect(subject).to be_able_to(:create, Comment) } # TODO returns false positive; needs work end - -# This is role based? to = array of team members etc shared_examples "can see mailings list too" do it { expect(subject).to be_able_to(:index, Mailing) } end - shared_examples "can read mailings sent to them" do let(:user) { create(:user) } it { expect(subject).to be_able_to(:read, Mailing, recipient: user )} end # Todo add shared_examples for other roles etc - From f6da595d771c2f7f8ed90699c799e95c40d07ed7 Mon Sep 17 00:00:00 2001 From: F3PiX Date: Thu, 17 May 2018 16:40:22 +0200 Subject: [PATCH 06/20] Cleanup file ; see previous commit for annotations --- app/controllers/conferences_controller.rb | 4 +- app/models/ability.rb | 24 +- spec/models/ability_spec.rb | 494 ++++------------------ spec/support/shared_examples/abilities.rb | 9 +- 4 files changed, 90 insertions(+), 441 deletions(-) diff --git a/app/controllers/conferences_controller.rb b/app/controllers/conferences_controller.rb index 7602599b3..b6da69f58 100644 --- a/app/controllers/conferences_controller.rb +++ b/app/controllers/conferences_controller.rb @@ -13,9 +13,7 @@ def show end def create - byebug - # page_to_redirect = current_user.admin? ? conferences_path : edit_team_path(current_student.current_team) - page_to_redirect = conferences_path + page_to_redirect = current_user.admin? ? conferences_path : edit_team_path(current_student.current_team) @conference = build_conference respond_to do |format| diff --git a/app/models/ability.rb b/app/models/ability.rb index ce12b83d3..87cbd7ba6 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -9,8 +9,7 @@ def initialize(user) alias_action :create, :read, :update, :destroy, to: :crud # guest user - can :read, Activity # pro forma ; Activity has no authorisation restriction, except for kind: :mailing - can :read, [User, Team, Project, Conference] + can :read, [Activity, User, Team, Project, Conference] return unless signed_in?(user) @@ -26,12 +25,6 @@ def initialize(user) can :resend_confirmation_instruction, User, id: user.id can :create, Project can [:join, :create], Team - # the validation is tested in spec/models/team_spec.rb:33 - # Should this be in ability at all? - # can :crud, Team do |team| - # team.new_record? - # end - # => delete can :index, Mailing can :read, Mailing do |mailing| mailing.recipient? user @@ -42,10 +35,8 @@ def initialize(user) on_team?(user, team) end - # Add / split restrictions for current season? - # current_student - if user.current_student? # TODO is this the best check? + if user.current_student? # TODO is this a valid check? can :create, Team if user.teams.none? can :create, Conference end @@ -53,12 +44,6 @@ def initialize(user) # supervisor if user.supervisor? can :read, :users_info - # explanation for this simpler declaration: - # The unconfirmed user ^ above had this declaration: - # `can :read_email, User, hide_email: false` - # is defined for all users: all can read an email address that is not hidden - # Here, the hide_email attribute doesnt matter: a supervisor can read it anyway - # See specs added to check this behaviour can :read_email, User do |other_user| supervises?(other_user, user) end @@ -77,8 +62,7 @@ def initialize(user) cannot :create, User # this only happens through GitHub end - ################# OLD FILE, # = moved to or rewritten above ############ - # NOT everything moved yet # + ################# REMAININGS FROM OLD FILE, # = rewritten above ############ # can :crud, Team do |team| # user.admin? || signed_in?(user) && team.new_record? || on_team?(user, team) @@ -99,7 +83,7 @@ def initialize(user) # cannot :create, Team do |team| # on_team_for_season?(user, team.season) || !user.confirmed? # end - # todo join helpdesk team + # todo helpdesk team join can :join, Team do |team| team.helpdesk_team? and signed_in?(user) and user.confirmed? and not on_team?(user, team) end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 5970d47fe..79567add6 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -16,7 +16,7 @@ it_behaves_like "can not create new user" it_behaves_like "can not comment" it_behaves_like "has no access to other user's accounts" - it_behaves_like "can not read role restricted or owner restricted pages" # now: ApplicationDraft + it_behaves_like "can not read role restricted or owner restricted pages" it "does not have an account" do true end @@ -26,7 +26,7 @@ let(:user) { build_stubbed(:user, :unconfirmed) } it_behaves_like 'same as guest user' - it_behaves_like "can modify own account" # User should always be able to update email address and resend mail + it_behaves_like "can modify own account" end describe "Confirmed user" do @@ -57,10 +57,9 @@ let(:student_team) { build(:team, :in_current_season) } let(:second_team) { build(:team, :in_current_season) } before do - # create :student_role, team: student_team, user: user allow(user).to receive(:current_student?).and_return(true) end - # TODO Questions: is this ^ stub correct to test the ability only? + # TODO Question: is this ^ stub correct to test the ability only? it_behaves_like "same public features as confirmed user" it { expect(subject).to be_able_to(:create, Conference) } @@ -118,7 +117,7 @@ it_behaves_like "can not create new user" # it "has access to almost everything else" - # To name the most exclusive and sensitive + # Only test the most exclusive and the most sensitive: it { expect(subject).to be_able_to(:crud, Team) } it { expect(subject).to be_able_to([:read, :update, :destroy], User) } it { expect(subject).to be_able_to(:read_email, other_user) } @@ -130,443 +129,112 @@ ############# OLD SPECS ################ context 'ability' do - # context 'when a user is connected' do - # let!(:user) { create(:user) } - # let!(:team) { create(:team) } - # - # describe 'she/he is allowed to do everything on her/his account' do - # it { expect(ability).to be_able_to(:show, user) } - # it { expect(ability).not_to be_able_to(:create, User.new) } # this only happens through GitHub - # - # it { expect(ability).to be_able_to(:resend_confirmation_instruction, user) } - # end - # - # context 'when a user is admin' do - # let(:organizer_role) { create(:organizer_role, user: user) } - # before { allow(organizer_role).to receive(:admin?).and_return(true) } - # - # it "should be able to CRUD on anyone's account" do - # expect(subject).to be_able_to(:crud, organizer_role) - # end - # - # describe 'she/he is not allowed to CRUD on someone else account' do - # let(:other_user) { create(:user) } - # # But an admin should! show and crud - # xit { expect(ability).not_to be_able_to(:show, other_user) } - # end - # end - - - - # describe 'who is allowed to see email address in user profile' do - # - # # email address is hidden: admin, user's supervisor in current season (confirmed) - # # email address is not hidden: admin, confirmed user, user him/herself - # - # let(:other_user) { create(:user) } - # - # context 'when email address is hidden' do - # context 'when an admin' do - # before do - # allow(user).to receive(:admin?).and_return(true) - # allow(ability).to receive(:supervises?).with(other_user, user).and_return(false) - # end - # it 'allows to see hidden email address' do - # other_user.hide_email = true - # expect(ability).to be_able_to(:read_email, other_user) - # end - # end - # - # context "when user's supervisor in current season (confirmed)" do - # before do - # allow(user).to receive(:admin?).and_return(false) - # allow(user).to receive(:supervisor?).and_return(true) - # allow(ability).to receive(:supervises?).with(other_user, user).and_return(true) - # allow(user).to receive(:confirmed?).and_return(true) - # end - # it 'allows to see hidden email address' do - # other_user.hide_email = true - # expect(ability).to be_able_to(:read_email, other_user) - # end - # end - # end - - # context 'when email address is not hidden' do - # context 'when an admin' do - # before do - # allow(user).to receive(:admin?).and_return(true) - # allow(user).to receive(:confirmed?).and_return(false) - # end - # it 'allows to see not hidden email address' do - # other_user.hide_email = false - # expect(ability).to be_able_to(:read_email, other_user) - # end - # end - # - # context 'when a confirmed user' do - # before do - # allow(user).to receive(:admin?).and_return(false) - # allow(user).to receive(:confirmed?).and_return(true) - # end - # it 'allows to see not hidden email address' do - # other_user.hide_email = false - # expect(ability).to be_able_to(:read_email, other_user) - # end - # end - # - # context 'when the user him/herself' do - # it 'allows to see not hidden email address' do - # user.hide_email = false - # expect(ability).to be_able_to(:read_email, user) - # end - # end - # end - # end - - # describe 'who is disallowed to see email address in user profile' do - # - # # email address is hidden: not admin, not user's supervisor in current season - # # email address is not hidden: not admin, not confirmed user, not user him/herself - # - # let(:other_user) { create(:user) } - # - # context 'when email address is hidden' do - # context "when not an admin or user's supervisor in current season" do - # before do - # allow(user).to receive(:admin?).and_return(false) - # allow(ability).to receive(:supervises?).with(other_user, user).and_return(false) - # end - # it 'disallows to see hidden email address' do - # other_user.hide_email = true - # expect(ability).not_to be_able_to(:read_email, other_user) - # end - # end - # end - # - # context 'when email address is not hidden' do - # context 'when not an admin or confirmed user or user him/herself' do - # before do - # allow(user).to receive(:admin?).and_return(false) - # allow(user).to receive(:confirmed?).and_return(false) - # end - # # NOTE / TODO is this testing "can? read_email" properly? - # xit 'disallows to see not hidden email address' do - # other_user.hide_email = false - # expect(ability).not_to be_able_to(:read_email, other_user) - # end - # end - # end - # - # end - - - # context 'resend_confirmation_instruction' do - # let(:other_user) { create(:user) } - # - # describe 'a user can only resend her/his own confirmation' do - # it { expect(ability).to be_able_to(:resend_confirmation_instruction, user) } - # it { expect(ability).not_to be_able_to(:resend_confirmation_instruction, other_user) } - # end - # - # describe 'a admin can resend all confirmation tokens' do - # let!(:organizer_role) { create(:organizer_role, user: user) } - # it { expect(ability).to be_able_to(:resend_confirmation_instruction, other_user) } - # end - # end - - describe "team's students or admin should be able to mark preferences to a conference" do - context 'when user is a student from a team and try to update conference preferences' do - - let!(:conference_preference) { create(:conference_preference, :student_preference) } - let!(:user) { conference_preference.team.students.first } - - it 'allows marking of conference preference' do - expect(ability).to be_able_to(:crud, conference_preference) - end + describe "team's students or admin should be able to mark preferences to a conference" do + context 'when user is a student from a team and try to update conference preferences' do - context 'when user is admin' do - let!(:organiser_role) { create(:organizer_role, user: user)} - it "should be able to crud conference preference" do - expect(subject).to be_able_to(:crud, conference_preference) - end - end - end + let!(:conference_preference) { create(:conference_preference, :student_preference) } + let!(:user) { conference_preference.team.students.first } - context 'when different users' do - let!(:other_user) { create(:user)} - let!(:conference_preference) { create(:conference_preference, team: team)} - - xit { expect(ability).not_to be_able_to(:crud, other_user) } + it 'allows marking of conference preference' do + expect(ability).to be_able_to(:crud, conference_preference) + end + context 'when user is admin' do + let!(:organiser_role) { create(:organizer_role, user: user)} + it "should be able to crud conference preference" do + expect(subject).to be_able_to(:crud, conference_preference) + end end end - describe "just orga members, team's supervisor and team's students should be able to see offered conference for a team" do - let(:user) { create(:student, confirmed_at: Date.yesterday)} + context 'when different users' do + let!(:other_user) { create(:user)} + let!(:conference_preference) { create(:conference_preference, team: team)} + # not testing what it means to test + xit { expect(ability).not_to be_able_to(:crud, other_user) } + end + end - context 'when the user is an student of another team' do - it { expect(ability).not_to be_able_to(:see_offered_conferences, Team.new) } - end + describe "just orga members, team's supervisor and team's students should be able to see offered conference for a team" do + let(:user) { create(:student, confirmed_at: Date.yesterday)} - context 'when the user is a supervisor of another team' do - before do - allow(user).to receive(:supervisor?).and_return(true) - end - it { expect(ability).not_to be_able_to(:see_offered_conferences, Team.new) } - end + context 'when the user is an student of another team' do + it { expect(ability).not_to be_able_to(:see_offered_conferences, Team.new) } + end - context "when the user is a team's student" do - it { expect(ability).to be_able_to(:see_offered_conferences, Team.new(students: [user])) } - end + context 'when the user is a supervisor of another team' do + before do + allow(user).to receive(:supervisor?).and_return(true) + end + it { expect(ability).not_to be_able_to(:see_offered_conferences, Team.new) } + end - context "when the user is a team's supervisor" do - it { expect(ability).to be_able_to(:see_offered_conferences, Team.new(supervisors: [user])) } - end + context "when the user is a team's student" do + it { expect(ability).to be_able_to(:see_offered_conferences, Team.new(students: [user])) } + end - context 'when the user is an admin' do - before do - allow(user).to receive(:admin?).and_return(true) - end - it { expect(ability).to be_able_to(:see_offered_conferences, Team.new) } - end + context "when the user is a team's supervisor" do + it { expect(ability).to be_able_to(:see_offered_conferences, Team.new(supervisors: [user])) } end - # describe 'to read user info' do - # context 'if not an admin or supervisor' do - # before do - # allow(user).to receive(:admin?).and_return(false) - # allow(user).to receive(:supervisor?).and_return(false) - # end - # - # it { expect(ability).not_to be_able_to(:read, :users_info) } - # end - # - # context 'if an admin' do - # before do - # allow(user).to receive(:admin?).and_return(true) - # allow(user).to receive(:supervisor?).and_return(false) - # end - # - # it { expect(ability).to be_able_to(:read, :users_info) } - # end - # - # context 'if a supervisor' do - # before do - # allow(user).to receive(:admin?).and_return(false) - # allow(user).to receive(:supervisor?).and_return(true) - # end - # - # it { expect(ability).to be_able_to(:read, :users_info) } - # end - # - # end - - # describe 'access to mailings' do - # let!(:mailing) { create(:mailing) } - # let!(:user) { create(:student, confirmed_at: Date.yesterday) } - # - # context 'when user is a recipient' do - # it 'allows to read' do - # mailing.to = %w(students) - # expect(subject).to be_able_to :read, mailing - # end - # end - # - # context 'when user has nothing to do with the mailing' do - # it 'will not allow to read' do - # mailing.to = %w(coaches) - # expect(subject).not_to be_able_to :read, mailing - # end - # end - # end - - describe 'operations on teams' do - let(:team) { create :team } - - # context 'when user admin' do - # let(:user) { create :organizer } - # - # it 'allows crud' do - # expect(subject).to be_able_to :crud, team - # end - # end - - # context 'when not confirmed' do - # let!(:user) { create :student, confirmed_at: nil } - # let(:team) { create(:team) } - # - # it 'does not allow to create teams' do - # expect(subject).not_to be_able_to :create, Team.new - # end - # - # it 'does not allow to join teams' do - # expect(subject).not_to be_able_to :join, team - # end - # end - - context 'when user student' do - let!(:user) { create :student } - - # it 'allows crud on new team' do - # pending "fails with new abilities, new tests pass" - # expect(subject).to be_able_to :crud, Team.new - # end - # - # it 'allows crud on own team' do - # create :student_role, team: team, user: user - # expect(subject).to be_able_to :crud, team - # end - # - # it 'does not allow crud on other team' do - # expect(subject).not_to be_able_to :crud, team - # end - - context 'when in team for season' do - before { create :student_role, team: student_team, user: user } - let(:student_team) { create(:team, :in_current_season) } - - # it 'allows crud on own team' do - # expect(subject).to be_able_to :crud, student_team - # end - - it 'allows update conferences on own team' do - expect(subject).to be_able_to :update_conference_preferences, student_team - end - - it 'does not allow update conference preferences for other teams' do - expect(subject).not_to be_able_to :update_conference_preferences, Team.new - end - - # it 'allows to create team for different season' do - # expect(subject).to be_able_to :create, Team.new - # end - - # it 'does not allow to create team for same season' do - # expect(subject).not_to be_able_to :create, Team.new(season: student_team.season) - # end - end + context 'when the user is an admin' do + before do + allow(user).to receive(:admin?).and_return(true) end - # context 'when user guest (not persisted)' do - # let(:user) { build :user } - # - # it 'does not allow crud on existing team' do - # expect(subject).not_to be_able_to :crud, team - # end - # - # it 'does not allow to create team' do - # expect(subject).not_to be_able_to :create, Team.new - # end - # end + it { expect(ability).to be_able_to(:see_offered_conferences, Team.new) } end + end - context 'permitting activities' do - # context 'for feed_entries' do - # it 'allows anyone to read' do - # expect(Ability.new(nil)).to be_able_to :read, Activity, kind: :feed_entry - # end - # end - + describe 'operations on teams' do + let(:team) { create :team } + context 'when user student' do + let!(:user) { create :student } - # context 'for mailings' do - # it 'does not allow anonymous user to read' do - # expect(Ability.new(nil)).not_to be_able_to :index, :mailing - # end - # it 'allows signed in user to read' do - # expect(subject).to be_able_to :index, Mailing - # end - # end - end + context 'when in team for season' do + before { create :student_role, team: student_team, user: user } + let(:student_team) { create(:team, :in_current_season) } - # end - - context 'working with projects' do - let!(:user) { create(:user) } + it 'allows update conferences on own team' do + expect(subject).to be_able_to :update_conference_preferences, student_team + end - # context 'crud' do - # - # # it 'can be edited when I am an admin' do - # # create(:organizer_role, user: user) - # # expect(subject).to be_able_to :crud, Project.new - # # end - # - # # it 'can be edited if I am the project submitter' do - # # expect(subject).to be_able_to :crud, Project.new(submitter: user) - # # end - # - # # it 'cannot be edited if my account is not confirmed' do - # # user.confirmed_at = nil - # # user.save - # # expect(subject).not_to be_able_to :crud, Project.new(submitter: user) - # # end - # - # end - - # context 'create' do - # # it 'can be created if I am confirmed' do - # # expect(subject).to be_able_to :create, Project.new - # # end - # - # # it 'cannot be created if I am not confirmed' do - # # user.confirmed_at = nil - # # user.save - # # expect(subject).not_to be_able_to :create, Project.new - # # end - # - # end - - # context 'when using a project as a template' do - # let(:user) { build :user } - # - # context 'for the original project submitter' do - # let(:project) { build :project, submitter: user } - # # it { is_expected.to be_able_to :use_as_template, project } - # - # context 'for a project from the same season' do - # let(:project) { build :project, :in_current_season, submitter: user } - # it { is_expected.not_to be_able_to :use_as_template, project } - # end - # end - # - # context 'for a project submitted by someone else' do - # let(:project) { build :project } - # it { is_expected.not_to be_able_to :use_as_template, project } - # end - # end + it 'does not allow update conference preferences for other teams' do + expect(subject).not_to be_able_to :update_conference_preferences, Team.new + end + end + end end - end - context 'to join helpdesk team' do - let(:user) { create(:helpdesk) } - let(:team) { create(:team) } - it 'should be logged in' do - expect(ability.signed_in?(user)).to eql true - end + context 'to join helpdesk team' do + let(:user) { create(:helpdesk) } + let(:team) { create(:team) } - it 'should not be part of existing team' do - expect(ability.on_team?(user, team)).to eql false - end + it 'should not be part of existing team' do + expect(ability.on_team?(user, team)).to eql false + end - it 'should be able to join helpdesk team' do - helpdesk_team = create(:team, :helpdesk) - expect(ability).to be_able_to(:join, helpdesk_team) + it 'should be able to join helpdesk team' do + helpdesk_team = create(:team, :helpdesk) + expect(ability).to be_able_to(:join, helpdesk_team) + end end - end - context 'Crud Conferences' do - let!(:user) { create(:user) } + context 'Crud Conferences' do + let!(:user) { create(:user) } - it 'permit crud conference when user is a current student' do - create :student_role, user: user - # but they shouldn't be able to crud? Only create. - # expect(ability).to be_able_to(:crud, Conference.new) - expect(ability).to be_able_to(:create, Conference.new) - end + it 'permit crud conference when user is a current student' do + create :student_role, user: user + # but they shouldn't be able to crud? Only create. + # expect(ability).to be_able_to(:crud, Conference.new) + expect(ability).to be_able_to(:create, Conference.new) + end - it 'permit crud conference when user is an organizer' do - create :organizer_role, user: user - expect(ability).to be_able_to(:crud, Conference.new) + it 'permit crud conference when user is an organizer' do + create :organizer_role, user: user + expect(ability).to be_able_to(:crud, Conference.new) + end end end end diff --git a/spec/support/shared_examples/abilities.rb b/spec/support/shared_examples/abilities.rb index ad589c179..520b6e888 100644 --- a/spec/support/shared_examples/abilities.rb +++ b/spec/support/shared_examples/abilities.rb @@ -29,7 +29,7 @@ end -# DETAILS , in order of appearance +# DETAILS, in order of appearance PUBLIC_INDEX_PAGES = [Activity, User, Team, Project, Conference].freeze @@ -38,7 +38,6 @@ PUBLIC_INDEX_PAGES.each do |page| it { expect(subject).to be_able_to(:read, page) } end - it { expect(subject).to be_able_to(:index, Activity, kind: :feed_entry) } # TODO delete it { expect(subject).not_to be_able_to(:index, Activity.with_kind(:mailing)) } it { expect(subject).to be_able_to(:read, User) } end @@ -53,7 +52,7 @@ it { expect(subject).to be_able_to(:create, Project) } end -shared_examples 'can not create new user' do +shared_examples "can not create new user" do it { expect(subject).not_to be_able_to(:create, User.new) } end @@ -67,7 +66,7 @@ end shared_examples "can not read role restricted or owner restricted pages" do - it { expect(subject).not_to be_able_to(:read, Application, ApplicationDraft) } # todo, collect all pages + it { expect(subject).not_to be_able_to(:read, Application, ApplicationDraft) } # todo, collect all pages end # details for logged in @@ -81,7 +80,7 @@ # details for confirmed shared_examples "can comment now" do - xit { expect(subject).to be_able_to(:create, Comment) } # TODO returns false positive; needs work + xit { expect(subject).to be_able_to(:create, Comment) } # TODO returns false positives and false negatives; needs work end shared_examples "can see mailings list too" do From b172a3fbf3f777259aa61991da5c183bae3f6cd3 Mon Sep 17 00:00:00 2001 From: F3PiX Date: Thu, 17 May 2018 17:41:32 +0200 Subject: [PATCH 07/20] hot fix for failing spec MailingsController (because of wrong authorisation) --- app/controllers/mailings_controller.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/mailings_controller.rb b/app/controllers/mailings_controller.rb index 9f3db104f..a62389997 100644 --- a/app/controllers/mailings_controller.rb +++ b/app/controllers/mailings_controller.rb @@ -1,11 +1,10 @@ # frozen_string_literal: true class MailingsController < ApplicationController - load_and_authorize_resource except: :index + load_and_authorize_resource def index @mailings = Mailing.order('id DESC').page(params[:page]) - authorize! :read, :mailing end # These actions are here to enable the cancancan 'not authorised' notice From ec3c03713e9e14d00bd3495271c0bf214f1fe017 Mon Sep 17 00:00:00 2001 From: F3PiX Date: Fri, 18 May 2018 14:14:05 +0200 Subject: [PATCH 08/20] Finetuning - remove redundant conditional --- app/models/ability.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 87cbd7ba6..f89bc493c 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -18,7 +18,7 @@ def initialize(user) can :resend_confirmation_instruction, User, id: user.id can :read_email, User, hide_email: false # view helper # delete? only used once - return unless user.confirmed? && signed_in?(user) + return unless user.confirmed? # confirmed user can [:update, :destroy], User, id: user.id From f84460e0b6ffab3cd128d6cefc76ded69f05daaa Mon Sep 17 00:00:00 2001 From: F3PiX Date: Fri, 18 May 2018 14:14:32 +0200 Subject: [PATCH 09/20] Finetuning - remove redundant things --- spec/factories/users.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 2379360f5..acd0a8abc 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -86,7 +86,7 @@ end trait :unconfirmed do - confirmed_at { nil } + confirmed_at nil end end end From dad17d8b0bd6bf4843db3dde578ae2042119c55f Mon Sep 17 00:00:00 2001 From: F3PiX Date: Fri, 18 May 2018 18:01:19 +0200 Subject: [PATCH 10/20] Write tests for desired behaviour of student ability whether or not to be able to create a new team -PM rspec green --- app/models/ability.rb | 2 +- spec/factories/season.rb | 4 +++ spec/models/ability_spec.rb | 56 ++++++++++++++++++++++--------------- 3 files changed, 38 insertions(+), 24 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index f89bc493c..815ffd895 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -36,7 +36,7 @@ def initialize(user) end # current_student - if user.current_student? # TODO is this a valid check? + if user.student? can :create, Team if user.teams.none? can :create, Conference end diff --git a/spec/factories/season.rb b/spec/factories/season.rb index 7db48d5bb..4b75bb31b 100644 --- a/spec/factories/season.rb +++ b/spec/factories/season.rb @@ -2,4 +2,8 @@ factory :season do sequence(:name, '2000') end + + trait :past do + name '2010' + end end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 79567add6..2a7c9abc9 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -17,9 +17,6 @@ it_behaves_like "can not comment" it_behaves_like "has no access to other user's accounts" it_behaves_like "can not read role restricted or owner restricted pages" - it "does not have an account" do - true - end end describe "Logged in user, not confirmed" do @@ -40,11 +37,12 @@ it { expect(subject).to be_able_to([:join, :create], Team) } describe "Team" do - describe 'All members' do - let(:current_team) { create(:team, :in_current_season) } - let(:other_team) { build_stubbed(:team, :in_current_season) } - let(:future_team) { build(:team, season: Season.succ )} + let(:current_team) { create(:team, :in_current_season) } + let(:other_team) { build_stubbed(:team, :in_current_season) } + let(:future_team) { build(:team, season: Season.succ )} + let(:old_team) { build_stubbed(:team, season: create(:season, :past)) } + describe 'All members' do # must be true for all roles but student; can that be stubbed or something? before { create :coach_role, team: current_team, user: user } @@ -54,26 +52,38 @@ end describe "Student" do - let(:student_team) { build(:team, :in_current_season) } - let(:second_team) { build(:team, :in_current_season) } - before do - allow(user).to receive(:current_student?).and_return(true) - end - # TODO Question: is this ^ stub correct to test the ability only? + let(:new_team) {build(:team, :in_current_season)} - it_behaves_like "same public features as confirmed user" - it { expect(subject).to be_able_to(:create, Conference) } + # TODO FIX BUG [reveiled by PR 997] + # the test for creating a second team is passing in team_spec because the validation kicks in on :submit + # The ability _should_ prevent access to the new form in the first place. + # There are some things wonky with the cancancan implementation. + # To be fixed in a separate issue + # These tests show the intended behaviour after the fix. - it "can create their first team" do - expect(subject).to be_able_to(:create, student_team) - end - it 'cannot create a second team in current_season' do - pending 'fails; it is tested in team_spec.rb:33, and that passes' - expect(subject).not_to be_able_to(:create, second_team) + context "with a student role from an earlier season" do + before {create :student_role, team: old_team, user: user} + + it "can create their first team" do + expect(subject).to be_able_to(:create, new_team) + end end - let(:future_team) { build(:team, season: Season.succ ) } - it { expect(subject).to be_able_to :create, future_team } + context "with a student role in the current season" do + before { create :student_role, team: current_team, user: user } + + it 'cannot create a second team in current_season' do + pending 'fails; it is tested in team_spec.rb:33, and that passes. See bug description above ^' + expect(subject).not_to be_able_to(:create, new_team) + end + + it 'can create a team for next season' do + expect(subject).to be_able_to :create, future_team + end + + it_behaves_like "same public features as confirmed user" + it { expect(subject).to be_able_to(:create, Conference) } + end end describe "Supervisor" do From ff1e8ba9b53a76b0a896c3e4497ceb2e40d4d3c3 Mon Sep 17 00:00:00 2001 From: F3PiX Date: Fri, 18 May 2018 19:45:46 +0200 Subject: [PATCH 11/20] Write tests for desired behaviour of whether a supervisor can or cannot read a user's email address when it's marked as hidden -PM rspec green --- spec/models/ability_spec.rb | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 2a7c9abc9..fafda2c42 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -88,21 +88,38 @@ describe "Supervisor" do let(:other_user) { create(:user) } - let(:other_team_user) { build_stubbed(:user) } + let(:other_team_user) { create(:user) } before do allow(user).to receive(:supervisor?).and_return(true) - allow(ability).to receive(:supervises?) { false } # Rspec complained: "stub default value first" - allow(ability).to receive(:supervises?).with(other_user, user).and_return(true) - allow(other_user).to receive(:hide_email).and_return(true) - allow(other_team_user).to receive(:hide_email).and_return(true) end - it_behaves_like "same public features as confirmed user" - it { expect(subject).to be_able_to(:read, :users_info, other_user) } - it { expect(subject).to be_able_to(:read, :users_info, other_team_user) } # but should they? - it { expect(subject).to be_able_to(:read_email, other_user) } - it { expect(subject).not_to be_able_to(:read_email, other_team_user )} + context "when viewing user information" do + + # FIXME Implementation + # It turns out that the `can? :read users_info` and `can? :read_email` are both view helpers + # (And they overlap) + # With the correct implementation of Cancancan, they are redundant. + # See issue #1001 + # These new specs should pass with the correct implementation + + it_behaves_like "same public features as confirmed user" + + before do + other_user.update!(hide_email: true) + other_team_user.update!(hide_email: true) + end + + it "can read email address of team members in current season, even when hidden" do + pending 'fails; fix in separate issue' + expect(subject).to receive(:supervises?).with(other_user, user).and_return true + expect(subject).to be_able_to(:read, other_user.email) + end + + it 'cannot read email of other users' do + expect(subject).not_to be_able_to(:read, other_team_user.email) + end + end end end From 975877fd178a0ba54a133fd04844dba61d3f380e Mon Sep 17 00:00:00 2001 From: F3PiX Date: Fri, 18 May 2018 19:54:22 +0200 Subject: [PATCH 12/20] Restore old code for can read_email and can users_info. Add tests for desired behaviour - Added new specs to described the intended behaviour whether a supervisor should or should not read hidden email addresses. - PM rspec green --- app/models/ability.rb | 19 ++++++++++++------- spec/models/ability_spec.rb | 2 +- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 815ffd895..bc27c0040 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -16,7 +16,7 @@ def initialize(user) # unconfirmed, logged in user can :update, User, id: user.id can :resend_confirmation_instruction, User, id: user.id - can :read_email, User, hide_email: false # view helper # delete? only used once + can :read_email, User, hide_email: false return unless user.confirmed? @@ -42,12 +42,7 @@ def initialize(user) end # supervisor - if user.supervisor? - can :read, :users_info - can :read_email, User do |other_user| - supervises?(other_user, user) - end - end + # Use old code # project submitter can [:update, :destroy], Project, submitter_id: user.id @@ -64,6 +59,16 @@ def initialize(user) ################# REMAININGS FROM OLD FILE, # = rewritten above ############ + # FIXME Leave this in until issue #1001 is fixed + # visibility of email address in user profile + can :read_email, User, id: user.id if !user.hide_email? + can :read_email, User if user.admin? + can :read_email, User do |other_user| + user.confirmed? && (supervises?(other_user, user) || !other_user.hide_email?) + end + can :read, :users_info if user.admin? || user.supervisor? + + # can :crud, Team do |team| # user.admin? || signed_in?(user) && team.new_record? || on_team?(user, team) # end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index fafda2c42..40929ea6f 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -112,7 +112,7 @@ it "can read email address of team members in current season, even when hidden" do pending 'fails; fix in separate issue' - expect(subject).to receive(:supervises?).with(other_user, user).and_return true + allow(subject).to receive(:supervises?).with(other_user, user).and_return true expect(subject).to be_able_to(:read, other_user.email) end From 1e5db7d0ec974d8c85de13a2a824883c45978dba Mon Sep 17 00:00:00 2001 From: F3PiX Date: Fri, 18 May 2018 23:19:38 +0200 Subject: [PATCH 13/20] WIP Split the ability specs in separate files per role - thanks to @klappradla -PM rspec green --- spec/models/ability/admin_spec.rb | 25 +++ spec/models/ability/confirmed_spec.rb | 22 +++ spec/models/ability/guest_spec.rb | 22 +++ .../models/ability/project_maintainer_spec.rb | 26 +++ spec/models/ability/team_member_spec.rb | 107 ++++++++++++ spec/models/ability/unconfirmed_spec.rb | 20 +++ spec/models/ability_spec.rb | 152 +----------------- 7 files changed, 226 insertions(+), 148 deletions(-) create mode 100644 spec/models/ability/admin_spec.rb create mode 100644 spec/models/ability/confirmed_spec.rb create mode 100644 spec/models/ability/guest_spec.rb create mode 100644 spec/models/ability/project_maintainer_spec.rb create mode 100644 spec/models/ability/team_member_spec.rb create mode 100644 spec/models/ability/unconfirmed_spec.rb diff --git a/spec/models/ability/admin_spec.rb b/spec/models/ability/admin_spec.rb new file mode 100644 index 000000000..d4e6ce980 --- /dev/null +++ b/spec/models/ability/admin_spec.rb @@ -0,0 +1,25 @@ +require 'rails_helper' +require 'cancan/matchers' + +# Run this file with +# $ rspec spec/models/ability_spec.rb -fd +# to see the output of specs running inside the shared examples [mdv] +RSpec.describe Ability, type: :model do + + let(:admin) { create(:user) } + subject(:ability) { Ability.new(admin) } + + let(:other_user) { build_stubbed(:user, hide_email: true) } + + describe "Admin" do + before { allow(admin).to receive(:admin?).and_return true } + + it_behaves_like "can not create new user" + # it "has access to almost everything else" + # Only test the most exclusive, the most sensitive and the 'cannots': + it { expect(subject).to be_able_to(:crud, Team) } + it { expect(subject).to be_able_to([:read, :update, :destroy], User) } + it { expect(subject).to be_able_to(:read_email, other_user) } + it { expect(subject).to be_able_to(:read, :users_info, other_user) } + end +end diff --git a/spec/models/ability/confirmed_spec.rb b/spec/models/ability/confirmed_spec.rb new file mode 100644 index 000000000..2d5dd133c --- /dev/null +++ b/spec/models/ability/confirmed_spec.rb @@ -0,0 +1,22 @@ +require 'rails_helper' +require 'cancan/matchers' + +# Run this file with +# $ rspec spec/models/ability_spec.rb -fd +# to see the output of specs running inside the shared examples [mdv] +RSpec.describe Ability, type: :model do + + let(:user) { create(:user) } + subject(:ability) { Ability.new(user) } + + describe "Confirmed user" do + + it_behaves_like "same as logged in user" + it_behaves_like "can create a Project" + it_behaves_like "can see mailings list too" + it_behaves_like "can read mailings sent to them" + it_behaves_like "can comment now" # not implemented; false positives; needs work + it { expect(subject).to be_able_to([:join, :create], Team) } + + end +end diff --git a/spec/models/ability/guest_spec.rb b/spec/models/ability/guest_spec.rb new file mode 100644 index 000000000..c97a03709 --- /dev/null +++ b/spec/models/ability/guest_spec.rb @@ -0,0 +1,22 @@ +require 'rails_helper' +require 'cancan/matchers' + +# Run this file with +# $ rspec spec/models/ability_spec.rb -fd +# to see the output of specs running inside the shared examples [mdv] +RSpec.describe Ability, type: :model do + + let(:user){ nil } + subject(:ability) { Ability.new(user) } + + let(:other_user) { build_stubbed(:user) } + + describe "Guest User" do + it_behaves_like "can view public pages" + it_behaves_like "can not modify things on public pages" + it_behaves_like "can not create new user" + it_behaves_like "can not comment" + it_behaves_like "has no access to other user's accounts" + it_behaves_like "can not read role restricted or owner restricted pages" + end +end diff --git a/spec/models/ability/project_maintainer_spec.rb b/spec/models/ability/project_maintainer_spec.rb new file mode 100644 index 000000000..70e7c1995 --- /dev/null +++ b/spec/models/ability/project_maintainer_spec.rb @@ -0,0 +1,26 @@ +require 'rails_helper' +require 'cancan/matchers' + +# Run this file with +# $ rspec spec/models/ability_spec.rb -fd +# to see the output of specs running inside the shared examples [mdv] + +RSpec.describe Ability, type: :model do + + describe "Project Submitter" do + + let(:user) { build_stubbed(:user) } + subject(:ability) { Ability.new(user) } + + let(:old_project) { build_stubbed(:project, submitter: user) } + let(:other_project) { build_stubbed(:project, submitter: other_user) } + let(:same_season_project) { build :project, :in_current_season, submitter: user } + let(:other_user) { build_stubbed(:user) } + + it_behaves_like "same public features as confirmed user" + it { expect(subject).to be_able_to([:update, :destroy], Project.new(submitter: user)) } + it { expect(subject).to be_able_to(:use_as_template, old_project) } + it { expect(subject).not_to be_able_to(:use_as_template, other_project) } + it { expect(subject).not_to be_able_to :use_as_template, same_season_project } + end +end diff --git a/spec/models/ability/team_member_spec.rb b/spec/models/ability/team_member_spec.rb new file mode 100644 index 000000000..515be563b --- /dev/null +++ b/spec/models/ability/team_member_spec.rb @@ -0,0 +1,107 @@ +require 'rails_helper' +require 'cancan/matchers' + +# Run this file with +# $ rspec spec/models/ability_spec.rb -fd +# to see the output of specs running inside the shared examples [mdv] +RSpec.describe Ability, type: :model do + + let(:user) { create(:user) } + subject(:ability) { Ability.new(user) } + + let(:other_user) { build_stubbed(:user, hide_email: true) } + + it_behaves_like "same as logged in user" + it_behaves_like "can create a Project" + it_behaves_like "can see mailings list too" + it_behaves_like "can read mailings sent to them" + it_behaves_like "can comment now" # not implemented; false positives; needs work + it { expect(subject).to be_able_to([:join, :create], Team) } + + describe "Team" do + let(:current_team) { create(:team, :in_current_season) } + let(:other_team) { build_stubbed(:team, :in_current_season) } + let(:future_team) { build(:team, season: Season.succ )} + let(:old_team) { build_stubbed(:team, season: create(:season, :past)) } + + describe 'All members' do + # must be true for all roles but student; can that be stubbed or something? + before { create :coach_role, team: current_team, user: user } + + it_behaves_like "same public features as confirmed user" + it { expect(subject).to be_able_to [:update, :destroy], current_team } + it { expect(subject).not_to be_able_to [:update, :destroy], other_team} + end + + describe "Student" do + let(:new_team) {build(:team, :in_current_season)} + + # TODO FIX BUG [reveiled by PR 997] + # the test for creating a second team is passing in team_spec because the validation kicks in on :submit + # The ability _should_ prevent access to the new form in the first place. + # There are some things wonky with the cancancan implementation. + # To be fixed in a separate issue + # These tests show the intended behaviour after the fix. + + context "with a student role from an earlier season" do + before {create :student_role, team: old_team, user: user} + + it "can create their first team" do + expect(subject).to be_able_to(:create, new_team) + end + end + + context "with a student role in the current season" do + before { create :student_role, team: current_team, user: user } + + it 'cannot create a second team in current_season' do + pending 'fails; it is tested in team_spec.rb:33, and that passes. See bug description above ^' + expect(subject).not_to be_able_to(:create, new_team) + end + + it 'can create a team for next season' do + expect(subject).to be_able_to :create, future_team + end + + it_behaves_like "same public features as confirmed user" + it { expect(subject).to be_able_to(:create, Conference) } + end + end + + describe "Supervisor" do + let(:other_user) { create(:user) } + let(:other_team_user) { create(:user) } + + before do + allow(user).to receive(:supervisor?).and_return(true) + end + + context "when viewing user information" do + + # FIXME Implementation + # It turns out that the `can? :read users_info` and `can? :read_email` are both view helpers + # (And they overlap) + # With the correct implementation of Cancancan, they are redundant. + # See issue #1001 + # These new specs should pass with the correct implementation + + it_behaves_like "same public features as confirmed user" + + before do + other_user.update!(hide_email: true) + other_team_user.update!(hide_email: true) + end + + it "can read email address of team members in current season, even when hidden" do + pending 'fails; fix in separate issue' + allow(subject).to receive(:supervises?).with(other_user, user).and_return true + expect(subject).to be_able_to(:read, other_user.email) + end + + it 'cannot read email of other users' do + expect(subject).not_to be_able_to(:read, other_team_user.email) + end + end + end + end +end diff --git a/spec/models/ability/unconfirmed_spec.rb b/spec/models/ability/unconfirmed_spec.rb new file mode 100644 index 000000000..79526cb11 --- /dev/null +++ b/spec/models/ability/unconfirmed_spec.rb @@ -0,0 +1,20 @@ +require 'rails_helper' +require 'cancan/matchers' + +# Run this file with +# $ rspec spec/models/ability_spec.rb -fd +# to see the output of specs running inside the shared examples [mdv] + +RSpec.describe Ability, type: :model do + + describe "User logged in, account unconfirmed" do + + let(:user){ build_stubbed(:user, :unconfirmed) } + subject(:ability) { Ability.new(user) } + + let(:other_user) { create(:user) } + + it_behaves_like 'same as guest user' + it_behaves_like "can modify own account" + end +end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 40929ea6f..cbc10e2e4 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -1,159 +1,14 @@ require 'rails_helper' require 'cancan/matchers' -# Run this file with -# $ rspec spec/models/ability_spec.rb -fd -# to see the output of specs running inside the shared examples [mdv] - RSpec.describe Ability, type: :model do + subject(:ability) { Ability.new(user) } let(:user){ nil } let(:other_user) { create(:user) } - describe "Guest User" do - it_behaves_like "can view public pages" - it_behaves_like "can not modify things on public pages" - it_behaves_like "can not create new user" - it_behaves_like "can not comment" - it_behaves_like "has no access to other user's accounts" - it_behaves_like "can not read role restricted or owner restricted pages" - end - - describe "Logged in user, not confirmed" do - let(:user) { build_stubbed(:user, :unconfirmed) } - - it_behaves_like 'same as guest user' - it_behaves_like "can modify own account" - end - - describe "Confirmed user" do - let!(:user) { create(:user) } - - it_behaves_like "same as logged in user" - it_behaves_like "can create a Project" - it_behaves_like "can see mailings list too" - it_behaves_like "can read mailings sent to them" - it_behaves_like "can comment now" # not implemented; false positives; needs work - it { expect(subject).to be_able_to([:join, :create], Team) } - - describe "Team" do - let(:current_team) { create(:team, :in_current_season) } - let(:other_team) { build_stubbed(:team, :in_current_season) } - let(:future_team) { build(:team, season: Season.succ )} - let(:old_team) { build_stubbed(:team, season: create(:season, :past)) } - - describe 'All members' do - # must be true for all roles but student; can that be stubbed or something? - before { create :coach_role, team: current_team, user: user } - - it_behaves_like "same public features as confirmed user" - it { expect(subject).to be_able_to [:update, :destroy], current_team } - it { expect(subject).not_to be_able_to [:update, :destroy], other_team} - end - - describe "Student" do - let(:new_team) {build(:team, :in_current_season)} - - # TODO FIX BUG [reveiled by PR 997] - # the test for creating a second team is passing in team_spec because the validation kicks in on :submit - # The ability _should_ prevent access to the new form in the first place. - # There are some things wonky with the cancancan implementation. - # To be fixed in a separate issue - # These tests show the intended behaviour after the fix. - - context "with a student role from an earlier season" do - before {create :student_role, team: old_team, user: user} - - it "can create their first team" do - expect(subject).to be_able_to(:create, new_team) - end - end - - context "with a student role in the current season" do - before { create :student_role, team: current_team, user: user } - - it 'cannot create a second team in current_season' do - pending 'fails; it is tested in team_spec.rb:33, and that passes. See bug description above ^' - expect(subject).not_to be_able_to(:create, new_team) - end - - it 'can create a team for next season' do - expect(subject).to be_able_to :create, future_team - end - - it_behaves_like "same public features as confirmed user" - it { expect(subject).to be_able_to(:create, Conference) } - end - end - - describe "Supervisor" do - let(:other_user) { create(:user) } - let(:other_team_user) { create(:user) } - - before do - allow(user).to receive(:supervisor?).and_return(true) - end - - context "when viewing user information" do - - # FIXME Implementation - # It turns out that the `can? :read users_info` and `can? :read_email` are both view helpers - # (And they overlap) - # With the correct implementation of Cancancan, they are redundant. - # See issue #1001 - # These new specs should pass with the correct implementation - - it_behaves_like "same public features as confirmed user" - - before do - other_user.update!(hide_email: true) - other_team_user.update!(hide_email: true) - end - - it "can read email address of team members in current season, even when hidden" do - pending 'fails; fix in separate issue' - allow(subject).to receive(:supervises?).with(other_user, user).and_return true - expect(subject).to be_able_to(:read, other_user.email) - end - - it 'cannot read email of other users' do - expect(subject).not_to be_able_to(:read, other_team_user.email) - end - end - end - end - - describe "Project Submitter" do - let(:old_project) { build_stubbed(:project, submitter: user) } - let(:other_project) { build_stubbed(:project, submitter: other_user) } - let(:same_season_project) { build :project, :in_current_season, submitter: user } - - it_behaves_like "same public features as confirmed user" - it { expect(subject).to be_able_to([:update, :destroy], Project.new(submitter: user)) } - it { expect(subject).to be_able_to(:use_as_template, old_project) } - it { expect(subject).not_to be_able_to(:use_as_template, other_project) } - it { expect(subject).not_to be_able_to :use_as_template, same_season_project } - end - end - - describe "Admin" do - let(:user) { create(:user) } - let(:other_user) { build_stubbed(:user, hide_email: true) } - - before { allow(user).to receive(:admin?) { true } } - - it_behaves_like "can not create new user" - # it "has access to almost everything else" - # Only test the most exclusive and the most sensitive: - it { expect(subject).to be_able_to(:crud, Team) } - it { expect(subject).to be_able_to([:read, :update, :destroy], User) } - it { expect(subject).to be_able_to(:read_email, other_user) } - it { expect(subject).to be_able_to(:read, :users_info, other_user) } - end - - - - ############# OLD SPECS ################ + ############# OLD SPECS ##################### + # TODO Reorganize to /ability/-*role*-specs # context 'ability' do describe "team's students or admin should be able to mark preferences to a conference" do @@ -255,6 +110,7 @@ create :student_role, user: user # but they shouldn't be able to crud? Only create. # expect(ability).to be_able_to(:crud, Conference.new) + # changed into: expect(ability).to be_able_to(:create, Conference.new) end From cabcae98c202fc20c07ce5250ef11e7e96e848bd Mon Sep 17 00:00:00 2001 From: F3PiX Date: Sat, 19 May 2018 00:10:54 +0200 Subject: [PATCH 14/20] WIP First try feature test guest user access --- spec/features/users/guest_user_spec.rb | 72 ++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 spec/features/users/guest_user_spec.rb diff --git a/spec/features/users/guest_user_spec.rb b/spec/features/users/guest_user_spec.rb new file mode 100644 index 000000000..50c1dddbc --- /dev/null +++ b/spec/features/users/guest_user_spec.rb @@ -0,0 +1,72 @@ +require 'rails_helper' + +RSpec.describe 'Guest User', type: :feature do + + let!(:activity) { create(:status_update, :published, team: team1) } + let!(:other_user) { create(:user) } + let!(:project) { create(:project, :in_current_season, :accepted, submitter: other_user) } + let!(:team1) { create(:team, name: 'Cheesy forever', project_id: project.id) } + let!(:out_of_season) { Season.current.starts_at - 1.week } + let!(:summer_season) { Season.current.starts_at + 1.week } + + context "when visiting public pages" do + + context 'All Year' do + before { Timecop.travel(out_of_season) } + after { Timecop.return } + + it 'can view Activities' do + visit root_path + expect(page).to have_css('h1', text: 'Activities') + find('.title', match: :smart).click + expect(page).to have_content(activity.title) + expect(page).to have_content('You must be logged in to add a comment.') + end + + it 'can view Community and User' do + visit community_path + expect(page).to have_css('h1', text: 'Community') + find_link(other_user.name, match: :smart).click + expect(page).to have_content("About me") + expect(page).to have_link("All participants") + expect(page).not_to have_link("Edit") # check + end + + it 'can view projects' do + visit projects_path + expect(page).to have_css('h1', text: 'Projects') # can be empty table + end + + it 'has a nav menu with public links' do + visit root_path + expect(page).to have_link("Activities") + find_link("Summer of Code").click + expect(page).to have_link("Teams") + expect(page).to have_link("Community") + expect(page).to have_link("Help") + end + + it 'has access to sign in link' do + visit root_path + expect(page).to have_link('Sign in') + end + end + + context 'in season' do + before do + Timecop.travel(summer_season) + end + after { Timecop.return } + + it "can view the current season's accepted and selected projects" do + # project not visible on page. why? + visit projects_path + expect(page).to have_css('h1', text: 'Projects') + # find_link(project.name, match: :smart).click + # expect(page).to have_content project.description + # expect(page).not_to have_link("Edit") + end + end + end + # continuing story in: sign_in_unconfirmed_user || sign_in_confirmed_user || sign_in_fail +end From 6e7d601c156e76c5e0012505417529f39eca558f Mon Sep 17 00:00:00 2001 From: F3PiX Date: Mon, 21 May 2018 00:03:19 +0200 Subject: [PATCH 15/20] FIX issue 1003; couldn't solve the abilities and pass the specs without solving the issue :-) - Unchanged: a student cannot create a second team in one season, and can create a team in next season - Fixed: a student in a team in current season doesn't get the '+ new team' button - Fixed: a student in a team in current season has no access to the new team form - Driving by: use the new rails 5 redirect-er to redirect back to the referer or fall back to root. (So that the forbidden access message appears on the page where the user tried something forbidden.) - Update projects_spec for new redirect - Update teams_controller_spec that failed because a student can't create a second team in the same season --- app/controllers/application_controller.rb | 2 +- app/controllers/teams_controller.rb | 2 +- app/models/ability.rb | 15 ++++------- app/views/teams/index.html.slim | 3 ++- spec/controllers/teams_controller_spec.rb | 22 ++++++++++----- spec/models/ability/team_member_spec.rb | 33 +++++++---------------- spec/requests/projects_spec.rb | 14 +++++----- 7 files changed, 42 insertions(+), 49 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index efa370fa7..06953056b 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -33,7 +33,7 @@ def after_sign_in_path_for(user) end rescue_from CanCan::AccessDenied do |exception| - redirect_to root_url, alert: exception.message + redirect_back(fallback_location: root_path, alert: exception.message) end def cors_preflight diff --git a/app/controllers/teams_controller.rb b/app/controllers/teams_controller.rb index 6273deb75..d9f6453d1 100644 --- a/app/controllers/teams_controller.rb +++ b/app/controllers/teams_controller.rb @@ -4,7 +4,7 @@ class TeamsController < ApplicationController before_action :set_users, only: [:new, :edit] before_action :set_display_roles, only: :index - load_and_authorize_resource except: [:index, :show] + authorize_resource except: [:index, :show] def index direction = params[:direction] == 'asc' ? 'ASC' : 'DESC' diff --git a/app/models/ability.rb b/app/models/ability.rb index bc27c0040..837da8bd6 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -37,12 +37,14 @@ def initialize(user) # current_student if user.student? - can :create, Team if user.teams.none? + cannot :create, Team do |team| + on_team_for_season?(user, team.season) + end can :create, Conference end # supervisor - # Use old code + # Use old code, see below # project submitter can [:update, :destroy], Project, submitter_id: user.id @@ -67,11 +69,7 @@ def initialize(user) user.confirmed? && (supervises?(other_user, user) || !other_user.hide_email?) end can :read, :users_info if user.admin? || user.supervisor? - - - # can :crud, Team do |team| - # user.admin? || signed_in?(user) && team.new_record? || on_team?(user, team) - # end + # can :update_conference_preferences, Team do |team| team.accepted? && team.students.include?(user) @@ -85,9 +83,6 @@ def initialize(user) team.students.include?(user) end - # cannot :create, Team do |team| - # on_team_for_season?(user, team.season) || !user.confirmed? - # end # todo helpdesk team join can :join, Team do |team| team.helpdesk_team? and signed_in?(user) and user.confirmed? and not on_team?(user, team) diff --git a/app/views/teams/index.html.slim b/app/views/teams/index.html.slim index 743430f36..454dcfa7b 100644 --- a/app/views/teams/index.html.slim +++ b/app/views/teams/index.html.slim @@ -1,7 +1,8 @@ nav.actions ul.list-inline li = link_to 'My Team', current_student.current_team, class: 'btn btn-default btn-sm' if current_student&.current_team - li = link_to icon('plus', 'New Team'), new_team_path, class: 'btn btn-primary btn-sm' if can? :create, Team.new(season: Season.current) + li = link_to icon('plus', 'New Team'), new_team_path, class: 'btn btn-primary btn-sm' if can? :create, Team + = 'Sign up to create your own team!' unless current_user div.pull-right.dropdown button.btn.btn-default.dropdown-toggle data-toggle="dropdown" | Past Teams diff --git a/spec/controllers/teams_controller_spec.rb b/spec/controllers/teams_controller_spec.rb index 162972c4f..c0c9c4b17 100644 --- a/spec/controllers/teams_controller_spec.rb +++ b/spec/controllers/teams_controller_spec.rb @@ -12,11 +12,11 @@ let(:valid_attributes) { build(:team).attributes.merge(roles_attributes: [{ name: 'coach', github_handle: 'tobias' }]) } - before do - user.roles.create(name: 'student', team: team) - end - describe "GET index" do + before do + user.roles.create(name: 'student', team: team) + end + context 'before acceptance letters are sent' do let(:last_season) { Season.create name: Date.today.year - 1 } let!(:invisble_team) { create :team, :in_current_season, kind: nil, invisible: true } @@ -103,6 +103,10 @@ end describe "GET edit" do + before do + user.roles.create(name: 'student', team: team) + end + context "their own team" do let(:team) { create(:team) } @@ -165,7 +169,10 @@ end describe "PATCH update" do - before { sign_in user } + before do + sign_in user + user.roles.create(name: 'student', team: team) + end context "their own team" do let(:team) { create(:team) } @@ -267,7 +274,10 @@ end describe "DELETE destroy" do - before { sign_in user } + before do + sign_in user + user.roles.create(name: 'student', team: team) + end context "their own team" do let(:params) { { id: team.to_param } } diff --git a/spec/models/ability/team_member_spec.rb b/spec/models/ability/team_member_spec.rb index 515be563b..da0c788a6 100644 --- a/spec/models/ability/team_member_spec.rb +++ b/spec/models/ability/team_member_spec.rb @@ -36,13 +36,6 @@ describe "Student" do let(:new_team) {build(:team, :in_current_season)} - # TODO FIX BUG [reveiled by PR 997] - # the test for creating a second team is passing in team_spec because the validation kicks in on :submit - # The ability _should_ prevent access to the new form in the first place. - # There are some things wonky with the cancancan implementation. - # To be fixed in a separate issue - # These tests show the intended behaviour after the fix. - context "with a student role from an earlier season" do before {create :student_role, team: old_team, user: user} @@ -55,7 +48,6 @@ before { create :student_role, team: current_team, user: user } it 'cannot create a second team in current_season' do - pending 'fails; it is tested in team_spec.rb:33, and that passes. See bug description above ^' expect(subject).not_to be_able_to(:create, new_team) end @@ -69,8 +61,8 @@ end describe "Supervisor" do - let(:other_user) { create(:user) } - let(:other_team_user) { create(:user) } + let(:team_member) { create(:user) } + let(:user_in_other_team) { create(:user) } before do allow(user).to receive(:supervisor?).and_return(true) @@ -78,28 +70,23 @@ context "when viewing user information" do - # FIXME Implementation - # It turns out that the `can? :read users_info` and `can? :read_email` are both view helpers - # (And they overlap) - # With the correct implementation of Cancancan, they are redundant. - # See issue #1001 - # These new specs should pass with the correct implementation - it_behaves_like "same public features as confirmed user" + # FIXME see issue #1001 + # The following specs should pass after fixing before do - other_user.update!(hide_email: true) - other_team_user.update!(hide_email: true) + team_member.update!(hide_email: true) + user_in_other_team.update!(hide_email: true) end it "can read email address of team members in current season, even when hidden" do - pending 'fails; fix in separate issue' - allow(subject).to receive(:supervises?).with(other_user, user).and_return true - expect(subject).to be_able_to(:read, other_user.email) + pending 'fails; To be solved with issue #1001' + allow(subject).to receive(:supervises?).with(team_member, user).and_return true + expect(subject).to be_able_to(:read, team_member.email) end it 'cannot read email of other users' do - expect(subject).not_to be_able_to(:read, other_team_user.email) + expect(subject).not_to be_able_to(:read, user_in_other_team.email) end end end diff --git a/spec/requests/projects_spec.rb b/spec/requests/projects_spec.rb index f54f6743c..bdbffc921 100644 --- a/spec/requests/projects_spec.rb +++ b/spec/requests/projects_spec.rb @@ -9,16 +9,16 @@ let(:season) { nil } let!(:project) { create :project, season: season, submitter: submitter } - shared_examples_for 'returns to the root page' do - it 'returns to the root page' do + shared_examples_for 'returns to the previous page' do + it 'returns to the previous page' do get use_as_template_project_path(project), headers: { 'HTTP_REFERER' => '/previouspage' } - expect(response).to redirect_to '/' # load_and_authorize_resource ignores the referrer + expect(response).to redirect_to '/previouspage' # load_and_authorize_resource finds the referrer expect(flash[:alert]).to match %r{not authorized} end end context 'when the user is not logged in' do - it_behaves_like 'returns to the root page' + it_behaves_like 'returns to the previous page' end context 'when the user is logged in' do @@ -26,7 +26,7 @@ before { sign_in user } context 'when the user does not own the project' do - it_behaves_like 'returns to the root page' + it_behaves_like 'returns to the previous page' end context 'when the user has submitted the project before' do @@ -34,13 +34,13 @@ context 'when the project is from the current season' do let(:season) { Season.current } - it_behaves_like 'returns to the root page' + it_behaves_like 'returns to the previous page' end context 'when the project is from a past season' do let(:season) { Season.find_or_create_by!(name: '2013') } - it 'returns to the root page' do + it 'returns to the previous page' do get use_as_template_project_path(project) expect(response).to be_success expect(response.body).to match project.name From 9a0f39bb6825a405c4998bd1c1e9e3f476b075ed Mon Sep 17 00:00:00 2001 From: F3PiX Date: Mon, 21 May 2018 10:17:20 +0200 Subject: [PATCH 16/20] Remove one level of nesting in the ability specs, removed the repition of testing role abilities. --- app/controllers/community_controller.rb | 1 + app/models/ability.rb | 7 +- spec/models/ability/admin_spec.rb | 2 +- spec/models/ability/confirmed_spec.rb | 22 ----- spec/models/ability/confirmed_user_spec.rb | 31 ++++++ spec/models/ability/guest_spec.rb | 16 ++-- .../models/ability/project_maintainer_spec.rb | 5 - spec/models/ability/team_member_spec.rb | 13 +-- spec/models/ability/unconfirmed_spec.rb | 20 ---- spec/models/ability/unconfirmed_user_spec.rb | 31 ++++++ spec/support/shared_examples/abilities.rb | 96 +++---------------- 11 files changed, 92 insertions(+), 152 deletions(-) delete mode 100644 spec/models/ability/confirmed_spec.rb create mode 100644 spec/models/ability/confirmed_user_spec.rb delete mode 100644 spec/models/ability/unconfirmed_spec.rb create mode 100644 spec/models/ability/unconfirmed_user_spec.rb diff --git a/app/controllers/community_controller.rb b/app/controllers/community_controller.rb index e96eb6bef..2571ddb8d 100644 --- a/app/controllers/community_controller.rb +++ b/app/controllers/community_controller.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class CommunityController < ApplicationController before_action :normalize_params, only: :index + authorize_resource :user, parent: false def index @filters = { diff --git a/app/models/ability.rb b/app/models/ability.rb index 837da8bd6..38ae00fe3 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -14,7 +14,7 @@ def initialize(user) return unless signed_in?(user) # unconfirmed, logged in user - can :update, User, id: user.id + can [:update, :destroy], User, id: user.id can :resend_confirmation_instruction, User, id: user.id can :read_email, User, hide_email: false @@ -29,6 +29,7 @@ def initialize(user) can :read, Mailing do |mailing| mailing.recipient? user end + can :create, Comment # Members in a team can [:update, :destroy], Team do |team| @@ -40,6 +41,7 @@ def initialize(user) cannot :create, Team do |team| on_team_for_season?(user, team.season) end + can :create, ApplicationDraft if user.application_drafts.in_current_season.none? can :create, Conference end @@ -104,9 +106,6 @@ def initialize(user) user.admin? || (preference.team.students.include? user) end - # applications - can :create, :application_draft if user.student? && user.application_drafts.in_current_season.none? - end # initializer def signed_in?(user) diff --git a/spec/models/ability/admin_spec.rb b/spec/models/ability/admin_spec.rb index d4e6ce980..de037fb40 100644 --- a/spec/models/ability/admin_spec.rb +++ b/spec/models/ability/admin_spec.rb @@ -14,7 +14,7 @@ describe "Admin" do before { allow(admin).to receive(:admin?).and_return true } - it_behaves_like "can not create new user" + it { expect(subject).not_to be_able_to(:create, User.new) } # happens only via GitHub # it "has access to almost everything else" # Only test the most exclusive, the most sensitive and the 'cannots': it { expect(subject).to be_able_to(:crud, Team) } diff --git a/spec/models/ability/confirmed_spec.rb b/spec/models/ability/confirmed_spec.rb deleted file mode 100644 index 2d5dd133c..000000000 --- a/spec/models/ability/confirmed_spec.rb +++ /dev/null @@ -1,22 +0,0 @@ -require 'rails_helper' -require 'cancan/matchers' - -# Run this file with -# $ rspec spec/models/ability_spec.rb -fd -# to see the output of specs running inside the shared examples [mdv] -RSpec.describe Ability, type: :model do - - let(:user) { create(:user) } - subject(:ability) { Ability.new(user) } - - describe "Confirmed user" do - - it_behaves_like "same as logged in user" - it_behaves_like "can create a Project" - it_behaves_like "can see mailings list too" - it_behaves_like "can read mailings sent to them" - it_behaves_like "can comment now" # not implemented; false positives; needs work - it { expect(subject).to be_able_to([:join, :create], Team) } - - end -end diff --git a/spec/models/ability/confirmed_user_spec.rb b/spec/models/ability/confirmed_user_spec.rb new file mode 100644 index 000000000..565f95573 --- /dev/null +++ b/spec/models/ability/confirmed_user_spec.rb @@ -0,0 +1,31 @@ +require 'rails_helper' +require 'cancan/matchers' + +# Run this file with +# $ rspec spec/models/ability_spec.rb -fd +# to see the output of specs running inside the shared examples [mdv] +RSpec.describe Ability, type: :model do + + let(:user) { build_stubbed(:user) } + subject(:ability) { Ability.new(user) } + let(:other_user) { build_stubbed(:user) } + + describe "Confirmed user" do + + it_behaves_like 'has access to public features' + + # same as unconfirmed: + it "can modify own account" do + expect(subject).to be_able_to([:update, :destroy], user) + expect(subject).to be_able_to(:resend_confirmation_instruction, User, id: user.id) + end + it { expect(subject).not_to be_able_to([:update, :destroy], other_user) } + + # the perks of confirming + it { expect(subject).to be_able_to([:join, :create], Team) } + it { expect(subject).to be_able_to(:create, Comment) } # TODO needs work for polymorphism + it { expect(subject).to be_able_to(:create, Project) } + it { expect(subject).to be_able_to(:index, Mailing) } + it { expect(subject).to be_able_to(:read, Mailing, recipient: user )} + end +end diff --git a/spec/models/ability/guest_spec.rb b/spec/models/ability/guest_spec.rb index c97a03709..03f433d18 100644 --- a/spec/models/ability/guest_spec.rb +++ b/spec/models/ability/guest_spec.rb @@ -12,11 +12,15 @@ let(:other_user) { build_stubbed(:user) } describe "Guest User" do - it_behaves_like "can view public pages" - it_behaves_like "can not modify things on public pages" - it_behaves_like "can not create new user" - it_behaves_like "can not comment" - it_behaves_like "has no access to other user's accounts" - it_behaves_like "can not read role restricted or owner restricted pages" + + it_behaves_like 'has access to public features' + + it "can not modify things on public pages" do + PUBLIC_INDEX_PAGES.each do |page| + expect(subject).not_to be_able_to([:create, :update, :destroy], page) + end + end + + it { expect(subject).not_to be_able_to(:create, Comment) } end end diff --git a/spec/models/ability/project_maintainer_spec.rb b/spec/models/ability/project_maintainer_spec.rb index 70e7c1995..190808dda 100644 --- a/spec/models/ability/project_maintainer_spec.rb +++ b/spec/models/ability/project_maintainer_spec.rb @@ -1,10 +1,6 @@ require 'rails_helper' require 'cancan/matchers' -# Run this file with -# $ rspec spec/models/ability_spec.rb -fd -# to see the output of specs running inside the shared examples [mdv] - RSpec.describe Ability, type: :model do describe "Project Submitter" do @@ -17,7 +13,6 @@ let(:same_season_project) { build :project, :in_current_season, submitter: user } let(:other_user) { build_stubbed(:user) } - it_behaves_like "same public features as confirmed user" it { expect(subject).to be_able_to([:update, :destroy], Project.new(submitter: user)) } it { expect(subject).to be_able_to(:use_as_template, old_project) } it { expect(subject).not_to be_able_to(:use_as_template, other_project) } diff --git a/spec/models/ability/team_member_spec.rb b/spec/models/ability/team_member_spec.rb index da0c788a6..91f423e7a 100644 --- a/spec/models/ability/team_member_spec.rb +++ b/spec/models/ability/team_member_spec.rb @@ -11,12 +11,8 @@ let(:other_user) { build_stubbed(:user, hide_email: true) } - it_behaves_like "same as logged in user" - it_behaves_like "can create a Project" - it_behaves_like "can see mailings list too" - it_behaves_like "can read mailings sent to them" - it_behaves_like "can comment now" # not implemented; false positives; needs work it { expect(subject).to be_able_to([:join, :create], Team) } + it { expect(subject).to be_able_to(:read, Mailing, recipient: user )} describe "Team" do let(:current_team) { create(:team, :in_current_season) } @@ -25,10 +21,8 @@ let(:old_team) { build_stubbed(:team, season: create(:season, :past)) } describe 'All members' do - # must be true for all roles but student; can that be stubbed or something? before { create :coach_role, team: current_team, user: user } - it_behaves_like "same public features as confirmed user" it { expect(subject).to be_able_to [:update, :destroy], current_team } it { expect(subject).not_to be_able_to [:update, :destroy], other_team} end @@ -55,7 +49,8 @@ expect(subject).to be_able_to :create, future_team end - it_behaves_like "same public features as confirmed user" + it { expect(subject).to be_able_to(:create, ApplicationDraft) } + it { expect(subject).to be_able_to(:create, Conference) } end end @@ -70,8 +65,6 @@ context "when viewing user information" do - it_behaves_like "same public features as confirmed user" - # FIXME see issue #1001 # The following specs should pass after fixing before do diff --git a/spec/models/ability/unconfirmed_spec.rb b/spec/models/ability/unconfirmed_spec.rb deleted file mode 100644 index 79526cb11..000000000 --- a/spec/models/ability/unconfirmed_spec.rb +++ /dev/null @@ -1,20 +0,0 @@ -require 'rails_helper' -require 'cancan/matchers' - -# Run this file with -# $ rspec spec/models/ability_spec.rb -fd -# to see the output of specs running inside the shared examples [mdv] - -RSpec.describe Ability, type: :model do - - describe "User logged in, account unconfirmed" do - - let(:user){ build_stubbed(:user, :unconfirmed) } - subject(:ability) { Ability.new(user) } - - let(:other_user) { create(:user) } - - it_behaves_like 'same as guest user' - it_behaves_like "can modify own account" - end -end diff --git a/spec/models/ability/unconfirmed_user_spec.rb b/spec/models/ability/unconfirmed_user_spec.rb new file mode 100644 index 000000000..e92877b05 --- /dev/null +++ b/spec/models/ability/unconfirmed_user_spec.rb @@ -0,0 +1,31 @@ +require 'rails_helper' +require 'cancan/matchers' + +# Run this file with +# $ rspec spec/models/ability_spec.rb -fd +# to see the output of specs running inside the shared examples [mdv] + +RSpec.describe Ability, type: :model do + + describe "User logged in, account unconfirmed" do + + let(:user){ create(:user, :unconfirmed) } + subject(:ability) { Ability.new(user) } + + it_behaves_like 'has access to public features' + + it "can not modify things on public pages" do + PUBLIC_INDEX_PAGES.each do |page| + expect(subject).not_to be_able_to([:create, :update, :destroy], page) + end + end + + it { expect(subject).not_to be_able_to(:create, Comment) } + + it "can modify own account" do + expect(subject).to be_able_to([:update], user) + expect(subject).to be_able_to(:destroy, user) + expect(subject).to be_able_to(:resend_confirmation_instruction, User, id: user.id) + end + end +end diff --git a/spec/support/shared_examples/abilities.rb b/spec/support/shared_examples/abilities.rb index 520b6e888..ac70e5026 100644 --- a/spec/support/shared_examples/abilities.rb +++ b/spec/support/shared_examples/abilities.rb @@ -1,95 +1,23 @@ -# collections of shared examples - -# logged in, unconfirmed: -shared_examples "same as guest user" do - it_behaves_like "can view public pages" - it_behaves_like "can not modify things on public pages" - it_behaves_like "can not create new user" - it_behaves_like "can not comment" - it_behaves_like "has no access to other user's accounts" - it_behaves_like "can not read role restricted or owner restricted pages" # now: ApplicationDraft -end - -# confirmed -shared_examples "same as logged in user" do - it_behaves_like "can view public pages" - it_behaves_like "can not create new user" - it_behaves_like "can not comment" - it_behaves_like "has no access to other user's accounts" - it_behaves_like "can not read role restricted or owner restricted pages" # now: ApplicationDraft - it_behaves_like "can modify own account" -end - -# different roles -shared_examples "same public features as confirmed user" do - it_behaves_like "can view public pages" - it_behaves_like "can not create new user" - it_behaves_like "can modify own account" - # todo can creates comments; needs work -end - - -# DETAILS, in order of appearance - PUBLIC_INDEX_PAGES = [Activity, User, Team, Project, Conference].freeze -# details for unrestricted -shared_examples 'can view public pages' do - PUBLIC_INDEX_PAGES.each do |page| - it { expect(subject).to be_able_to(:read, page) } - end - it { expect(subject).not_to be_able_to(:index, Activity.with_kind(:mailing)) } - it { expect(subject).to be_able_to(:read, User) } -end +# guest, unconfirmed and confirmed users without roles +shared_examples 'has access to public features' do + let(:other_user) { create(:user)} -shared_examples "can not modify things on public pages" do - PUBLIC_INDEX_PAGES.each do |page| - it { expect(subject).not_to be_able_to([:create, :update, :destroy], page) } + it 'has access to public pages' do + PUBLIC_INDEX_PAGES.each do |page| + expect(subject).to be_able_to(:read, page) + end + expect(subject).not_to be_able_to(:index, Activity.with_kind(:mailing)) + expect(subject).to be_able_to(:read, User) end -end -shared_examples "can create a Project" do - it { expect(subject).to be_able_to(:create, Project) } -end + it "has no access to role restricted or owner restricted pages" do + expect(subject).not_to be_able_to(:read, Application, ApplicationDraft) # todo, collect all pages + end -shared_examples "can not create new user" do it { expect(subject).not_to be_able_to(:create, User.new) } -end - -shared_examples "can not comment" do - it { expect(subject).not_to be_able_to([:create, :update, :destroy], Comment) } -end - -shared_examples "has no access to other user's accounts" do # pro memorie: outside their team - let(:other_user) { create(:user)} it { expect(subject).not_to be_able_to([:update, :destroy], other_user) } end -shared_examples "can not read role restricted or owner restricted pages" do - it { expect(subject).not_to be_able_to(:read, Application, ApplicationDraft) } # todo, collect all pages -end - -# details for logged in -shared_examples "can modify own account" do - let(:user) {create(:user) } - - it { expect(subject).to be_able_to([:update, :destroy], user) } - it { expect(subject).to be_able_to(:resend_confirmation_instruction, User, id: user.id) } -end - -# details for confirmed - -shared_examples "can comment now" do - xit { expect(subject).to be_able_to(:create, Comment) } # TODO returns false positives and false negatives; needs work -end - -shared_examples "can see mailings list too" do - it { expect(subject).to be_able_to(:index, Mailing) } -end - -shared_examples "can read mailings sent to them" do - let(:user) { create(:user) } - it { expect(subject).to be_able_to(:read, Mailing, recipient: user )} -end - # Todo add shared_examples for other roles etc From e765a2ba4d78b83ebf115fa1fa227a897a1ad7b1 Mon Sep 17 00:00:00 2001 From: F3PiX Date: Mon, 21 May 2018 10:20:41 +0200 Subject: [PATCH 17/20] :cop: --- spec/models/ability/admin_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/ability/admin_spec.rb b/spec/models/ability/admin_spec.rb index de037fb40..dad2afc4c 100644 --- a/spec/models/ability/admin_spec.rb +++ b/spec/models/ability/admin_spec.rb @@ -14,7 +14,7 @@ describe "Admin" do before { allow(admin).to receive(:admin?).and_return true } - it { expect(subject).not_to be_able_to(:create, User.new) } # happens only via GitHub + it { expect(subject).not_to be_able_to(:create, User.new) } # happens only via GitHub # it "has access to almost everything else" # Only test the most exclusive, the most sensitive and the 'cannots': it { expect(subject).to be_able_to(:crud, Team) } From 9d685da7aeed9045bf9d7de40e973bc782d2eb66 Mon Sep 17 00:00:00 2001 From: F3PiX Date: Mon, 21 May 2018 10:46:43 +0200 Subject: [PATCH 18/20] Revert changes to authorize method in controllers that are not needed in this PR --- app/controllers/community_controller.rb | 1 - app/controllers/teams_controller.rb | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/community_controller.rb b/app/controllers/community_controller.rb index 2571ddb8d..e96eb6bef 100644 --- a/app/controllers/community_controller.rb +++ b/app/controllers/community_controller.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true class CommunityController < ApplicationController before_action :normalize_params, only: :index - authorize_resource :user, parent: false def index @filters = { diff --git a/app/controllers/teams_controller.rb b/app/controllers/teams_controller.rb index d9f6453d1..6273deb75 100644 --- a/app/controllers/teams_controller.rb +++ b/app/controllers/teams_controller.rb @@ -4,7 +4,7 @@ class TeamsController < ApplicationController before_action :set_users, only: [:new, :edit] before_action :set_display_roles, only: :index - authorize_resource except: [:index, :show] + load_and_authorize_resource except: [:index, :show] def index direction = params[:direction] == 'asc' ? 'ASC' : 'DESC' From 419bdf767d92dacee35285764e459b9d49d027e7 Mon Sep 17 00:00:00 2001 From: F3PiX Date: Mon, 21 May 2018 10:54:50 +0200 Subject: [PATCH 19/20] Declutter --- spec/models/ability/team_member_spec.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/spec/models/ability/team_member_spec.rb b/spec/models/ability/team_member_spec.rb index 91f423e7a..857d6d810 100644 --- a/spec/models/ability/team_member_spec.rb +++ b/spec/models/ability/team_member_spec.rb @@ -1,9 +1,6 @@ require 'rails_helper' require 'cancan/matchers' -# Run this file with -# $ rspec spec/models/ability_spec.rb -fd -# to see the output of specs running inside the shared examples [mdv] RSpec.describe Ability, type: :model do let(:user) { create(:user) } From cb126b9f695a544f6f064c99deb2c098eea02209 Mon Sep 17 00:00:00 2001 From: F3PiX Date: Mon, 21 May 2018 18:58:19 +0200 Subject: [PATCH 20/20] Add scope to solve failing specs. --- spec/features/users/guest_user_spec.rb | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/spec/features/users/guest_user_spec.rb b/spec/features/users/guest_user_spec.rb index 50c1dddbc..1276c5aa3 100644 --- a/spec/features/users/guest_user_spec.rb +++ b/spec/features/users/guest_user_spec.rb @@ -5,7 +5,7 @@ let!(:activity) { create(:status_update, :published, team: team1) } let!(:other_user) { create(:user) } let!(:project) { create(:project, :in_current_season, :accepted, submitter: other_user) } - let!(:team1) { create(:team, name: 'Cheesy forever', project_id: project.id) } + let!(:team1) { create(:team, :in_current_season, name: 'Cheesy forever', project_name: project.name, project_id: project.id) } let!(:out_of_season) { Season.current.starts_at - 1.week } let!(:summer_season) { Season.current.starts_at + 1.week } @@ -59,12 +59,11 @@ after { Timecop.return } it "can view the current season's accepted and selected projects" do - # project not visible on page. why? visit projects_path expect(page).to have_css('h1', text: 'Projects') - # find_link(project.name, match: :smart).click - # expect(page).to have_content project.description - # expect(page).not_to have_link("Edit") + find_link(project.name, match: :smart).click + expect(page).to have_content project.description + expect(page).not_to have_link("Edit") end end end