From 9a4b217c52792a32ee3c225af6312a6b5221bb57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Rodrigues?= Date: Wed, 13 Sep 2017 09:37:09 -0300 Subject: [PATCH] Disallow guests from modifying project (#243) * don't show projects to guest users * disallow guests from modifying project * add items to changelog * Fix tests + rubocop issues * Guest user cannot see the add story button * Disable story attributes for guest user --- CHANGELOG.md | 4 ++++ app/assets/javascripts/models/story.js | 14 ++++++++--- app/assets/javascripts/views/story_view.js | 16 +++++++++++-- app/controllers/projects_controller.rb | 7 ++++-- app/models/user.rb | 6 ++++- app/policies/application_policy.rb | 2 ++ app/policies/project_policy.rb | 3 ++- app/policies/story_policy.rb | 4 ++++ app/views/projects/show.html.erb | 10 ++++---- spec/features/stories_spec.rb | 28 ++++++++++++++++++++-- spec/javascripts/models/story_spec.js | 3 ++- spec/models/user_spec.rb | 2 +- spec/policies/project_policy_spec.rb | 27 +++++++++++++++++++++ spec/policies/story_policy_spec.rb | 26 ++++++++++++++++++-- 14 files changed, 133 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e48bba11d..818409089 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ ### Fixed - Allowed params to update project via API +### Added +- Don't show projects to guests that he's not member of +- Disallow guests to make changes on projects + ## [1.9.0] 2017-09-11 ### Changed - Changing `webpack-rails` gem to new `webpacker` gem diff --git a/app/assets/javascripts/models/story.js b/app/assets/javascripts/models/story.js index 5710a3b24..e6396fc72 100644 --- a/app/assets/javascripts/models/story.js +++ b/app/assets/javascripts/models/story.js @@ -40,9 +40,17 @@ var Story = module.exports = Backbone.Model.extend({ this.setReadonly(); }, - setReadonly: function() { - if(this.get('state') === 'accepted' && this.get('accepted_at') !== undefined) - this.isReadonly = true; + setReadonly: function () { + var accepted = this.get('state') === 'accepted' && this.get('accepted_at') !== undefined; + var isGuest = ( + this.collection !== undefined && + this.collection.project.current_user !== undefined && + this.collection.project.current_user.get('guest?') + ); + + if (isGuest || accepted) { + this.isReadonly = true + } }, changeState: function(model, newValue) { diff --git a/app/assets/javascripts/views/story_view.js b/app/assets/javascripts/views/story_view.js index 89bfa0aa2..975bdab2a 100644 --- a/app/assets/javascripts/views/story_view.js +++ b/app/assets/javascripts/views/story_view.js @@ -343,6 +343,12 @@ module.exports = FormView.extend({ ReactDOM.unmountComponentAtNode(storyControlsContainer); } + var isGuest = ( + this.model.collection !== undefined && + this.model.collection.project.current_user !== undefined && + this.model.collection.project.current_user.get('guest?') + ); + if(this.canEdit()) { this.$el.empty(); @@ -424,9 +430,12 @@ module.exports = FormView.extend({ } this.renderReactComponents(); + if (isGuest) { this.toggleControlButtons(true, false) } + } else { this.$el.removeClass('editing'); this.$el.html(this.template({story: this.model, view: this})); + if (isGuest) { this.$el.find('.state-actions').find('.transition').prop('disabled', true) } } this.hoverBox(); return this; @@ -923,9 +932,12 @@ module.exports = FormView.extend({ })); }, - toggleControlButtons: function(isDisabled) { + toggleControlButtons: function(isDisabled, changeCancel) { var $storyControls = this.$el.find('.story-controls'); - $storyControls.find('.submit, .destroy, .cancel').prop('disabled', isDisabled); + $storyControls.find('.submit, .destroy').prop('disabled', isDisabled); + + if (changeCancel === undefined) { changeCancel = true } + if (changeCancel) { $storyControls.find('.cancel').prop('disabled', isDisabled); } }, getLocation: function() { diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index b5e948520..126e2f565 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -15,10 +15,13 @@ def index projects_joined = policy_scope(Project) @projects = { - joined: serialize_from_collection(projects_joined), - unjoined: serialize_from_collection(projects_unjoined.order(:updated_at)) + joined: serialize_from_collection(projects_joined) } + unless current_user.guest? + @projects[:unjoined] = serialize_from_collection(projects_unjoined.order(:updated_at)) + end + @activities_group = Activity.grouped_activities(projects_joined, 1.week.ago) end diff --git a/app/models/user.rb b/app/models/user.rb index 487295572..b6b790596 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -37,6 +37,10 @@ def password_required? end end + def guest? + role == 'guest' + end + def to_s "#{name} (#{initials}) <#{email}>" end @@ -56,7 +60,7 @@ def tour_steps end def as_json(_options = {}) - super(only: JSON_ATTRIBUTES, methods: :tour_steps) + super(only: JSON_ATTRIBUTES, methods: [:tour_steps, :guest?]) end def self.find_first_by_auth_conditions(warden_conditions) diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb index c5343f7d0..d07b010a1 100644 --- a/app/policies/application_policy.rb +++ b/app/policies/application_policy.rb @@ -11,6 +11,8 @@ def self.included(base) protected + delegate :guest?, to: :current_user + def root? # this user can do anothing, it goes in AdminUser instead of User and bypasses everything context.active_admin diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index c01e410f8..6f76744a9 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -12,7 +12,8 @@ def import? end def join? - !project_member? + return false if project_member? || guest? + true end alias archived? update? diff --git a/app/policies/story_policy.rb b/app/policies/story_policy.rb index c603d6731..7d5108d51 100644 --- a/app/policies/story_policy.rb +++ b/app/policies/story_policy.rb @@ -1,17 +1,21 @@ class StoryPolicy < ApplicationPolicy def index? + return false if guest? admin? || project_member? end def show? + return false if guest? admin? || project_member? && current_project.stories.find_by(id: record.id) end def create? + return false if guest? admin? || project_member? end def update? + return false if guest? admin? || project_member? end diff --git a/app/views/projects/show.html.erb b/app/views/projects/show.html.erb index 439db7d01..cbb99f5e2 100644 --- a/app/views/projects/show.html.erb +++ b/app/views/projects/show.html.erb @@ -14,10 +14,12 @@

- + <% unless current_user.guest? %> + + <% end %> <% end %> <% content_for :right_menu do %> diff --git a/spec/features/stories_spec.rb b/spec/features/stories_spec.rb index 90bd149a8..59fcd5e15 100644 --- a/spec/features/stories_spec.rb +++ b/spec/features/stories_spec.rb @@ -11,8 +11,32 @@ end describe 'full story life cycle' do - before do - project + context 'when the user is guest' do + let!(:story) do + create(:story, title: 'Test Story', project: project, requested_by: user, estimate: 2) + end + + before do + user.update_column(:role, 'guest') + visit project_path(project) + wait_spinner + wait_page_load + end + + it 'cannot see the add story button', js: true do + expect(page).to_not have_button 'Add story' + end + + it 'attributes are disabled', js: true do + expect(page).to have_button('start', disabled: true) + + find('.story-title').click + expect(page).to have_field('title', with: story.title, disabled: true) + expect(page).to have_select('story_type', selected: 'feature', disabled: true) + expect(page).to have_button('Save', disabled: true) + expect(page).to have_button('Delete', disabled: true) + expect(page).to have_button('Cancel', disabled: false) + end end it 'steps through the full story life cycle', js: true do diff --git a/spec/javascripts/models/story_spec.js b/spec/javascripts/models/story_spec.js index c36779073..e0e7acca9 100644 --- a/spec/javascripts/models/story_spec.js +++ b/spec/javascripts/models/story_spec.js @@ -1,4 +1,5 @@ var Story = require('models/story'); +var User = require('models/user'); describe('Story', function() { @@ -7,7 +8,7 @@ describe('Story', function() { name: 'project', defaults: {point_values: [0, 1, 2, 3]}, users: { get: function() {} }, - current_user: { id: 999 }, + current_user: new User({ id: 999, 'guest?': false }), currentIterationNumber: function() { return 1; }, getIterationNumberForDate: function() { return 999; } }); diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index cf1a99391..de2c20128 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -17,7 +17,7 @@ specify do expect(subject.as_json['user'].keys.sort).to eq( - %w(email finished_tour id initials name tour_steps username) + %w(email finished_tour guest? id initials name tour_steps username) ) end end diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index d9db54799..0eb9071fe 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -161,5 +161,32 @@ expect(policy_scope).to eq([]) end end + + context 'for a guest' do + let(:current_user) { create :user, :with_team, role: 'guest' } + + %i(index create new update edit reports).each do |action| + it { should_not permit(action) } + end + + %i( + import + import_upload + archive + unarchive + destroy + share + unshare + transfer + ownership + join + ).each do |action| + it { should_not permit(action) } + end + + it 'hides project' do + expect(policy_scope).to eq([]) + end + end end end diff --git a/spec/policies/story_policy_spec.rb b/spec/policies/story_policy_spec.rb index df557a135..0b5798976 100644 --- a/spec/policies/story_policy_spec.rb +++ b/spec/policies/story_policy_spec.rb @@ -35,8 +35,6 @@ context 'for a user' do let(:current_user) { create :user, :with_team } - it { should permit(:show) } - %i(index show create new update edit destroy).each do |action| it { should permit(action) } end @@ -45,6 +43,18 @@ expect(policy_scope).to eq([story]) end end + + context 'for a guest' do + let(:current_user) { create :user, :with_team, role: 'guest' } + + %i(index show create new update edit destroy).each do |action| + it { should_not permit(action) } + end + + it 'lists all stories of the project' do + expect(policy_scope).to eq([story]) + end + end end context 'user not a member of project' do @@ -71,5 +81,17 @@ expect(policy_scope).to eq([]) end end + + context 'for a guest' do + let(:current_user) { create :user, :with_team, role: 'guest' } + + %i(index create new update edit destroy).each do |action| + it { should_not permit(action) } + end + + it 'hides stories of project that is not member' do + expect(policy_scope).to eq([]) + end + end end end