From bd90f7e2deaf4caddfab9472bd129385de74e909 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 30 Sep 2019 14:07:50 -0400 Subject: [PATCH 1/9] Add policy for instance security dashboard So long as the right license is there, any logged in user should be able to view the dashboard. https://gitlab.com/gitlab-org/gitlab/issues/33831 --- ee/app/policies/ee/global_policy.rb | 6 ++++++ ee/spec/policies/global_policy_spec.rb | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/ee/app/policies/ee/global_policy.rb b/ee/app/policies/ee/global_policy.rb index 116478f4b29580..07abf65046d52a 100644 --- a/ee/app/policies/ee/global_policy.rb +++ b/ee/app/policies/ee/global_policy.rb @@ -9,7 +9,13 @@ module GlobalPolicy License.feature_available?(:operations_dashboard) end + condition(:security_dashboard_available) do + License.feature_available?(:security_dashboard) + end + rule { operations_dashboard_available }.enable :read_operations_dashboard + rule { security_dashboard_available }.enable :read_security_dashboard + rule { admin }.policy do enable :read_licenses enable :destroy_licenses diff --git a/ee/spec/policies/global_policy_spec.rb b/ee/spec/policies/global_policy_spec.rb index ca11d3fdd60a15..c847b3be50a0a9 100644 --- a/ee/spec/policies/global_policy_spec.rb +++ b/ee/spec/policies/global_policy_spec.rb @@ -55,4 +55,22 @@ describe 'view_productivity_analytics' do include_examples 'analytics policy', :view_productivity_analytics end + + describe 'read_security_dashboard' do + context 'when the instance has an Ultimate license' do + before do + stub_licensed_features(security_dashboard: true) + end + + it { is_expected.to be_allowed(:read_security_dashboard) } + end + + context 'when the instance does not have an Ultimate license' do + before do + stub_licensed_features(security_dashboard: false) + end + + it { is_expected.not_to be_allowed(:read_security_dashboard) } + end + end end -- GitLab From 892f7dcd90fc28728cd233e1240826d537d88e09 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 30 Sep 2019 15:44:01 -0400 Subject: [PATCH 2/9] Start pattern for dashboard controllers * SecurityController -> Security::DashboardController Having a namespace for the security dashboard controllers will allow us to better organize our resources. For example, the addition and removal of projects can take place in Security::ProjectsController instead of everything happening inside one SecurityController * For now, respond with `head :ok`. A future MR will implement the action bodies. --- .../security/dashboard_controller.rb | 21 +++++++++ ee/app/controllers/security_controller.rb | 13 ------ ee/config/routes/security.rb | 6 ++- .../security/dashboard_controller_spec.rb | 43 +++++++++++++++++++ 4 files changed, 69 insertions(+), 14 deletions(-) create mode 100644 ee/app/controllers/security/dashboard_controller.rb delete mode 100644 ee/app/controllers/security_controller.rb create mode 100644 ee/spec/controllers/security/dashboard_controller_spec.rb diff --git a/ee/app/controllers/security/dashboard_controller.rb b/ee/app/controllers/security/dashboard_controller.rb new file mode 100644 index 00000000000000..ae8b037e391b67 --- /dev/null +++ b/ee/app/controllers/security/dashboard_controller.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Security + class DashboardController < ApplicationController + before_action :authorize_read_security_dashboard! + before_action do + push_frontend_feature_flag(:security_dashboard) + end + + def show + head :ok + end + + private + + def authorize_read_security_dashboard! + render_404 unless Feature.enabled?(:security_dashboard) && + can?(current_user, :read_security_dashboard) + end + end +end diff --git a/ee/app/controllers/security_controller.rb b/ee/app/controllers/security_controller.rb deleted file mode 100644 index 95911c696c02f9..00000000000000 --- a/ee/app/controllers/security_controller.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -class SecurityController < ApplicationController - before_action :authorize_read_security_dashboard! - before_action do - push_frontend_feature_flag(:security_dashboard) - end - - def authorize_read_security_dashboard! - render_404 unless Feature.enabled?(:security_dashboard) && - can?(current_user, :read_security_dashboard) - end -end diff --git a/ee/config/routes/security.rb b/ee/config/routes/security.rb index c5a263bd95f5e4..8c38c793bc1f49 100644 --- a/ee/config/routes/security.rb +++ b/ee/config/routes/security.rb @@ -1,3 +1,7 @@ # frozen_string_literal: true -get 'security' => 'security#index' +namespace :security do + root to: 'dashboard#show' + + resources :projects, only: [:index, :create, :delete] +end diff --git a/ee/spec/controllers/security/dashboard_controller_spec.rb b/ee/spec/controllers/security/dashboard_controller_spec.rb new file mode 100644 index 00000000000000..e8ed3358348880 --- /dev/null +++ b/ee/spec/controllers/security/dashboard_controller_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Security::DashboardController do + include Rails.application.routes.url_helpers + + let(:user) { create(:user) } + + before do + stub_feature_flags(security_dashboard: true) + stub_licensed_features(security_dashboard: true) + sign_in(user) + end + + describe 'GET #show' do + it 'responds with success' do + get :show + + expect(response).to have_gitlab_http_status(:ok) + end + + context 'when the instance does not have an Ultimate license' do + it '404s' do + stub_licensed_features(security_dashboard: false) + + get :show + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when the security dashboard feature is disabled' do + it '404s' do + stub_feature_flags(security_dashboard: false) + + get :show + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end +end -- GitLab From 2a483931417ba446fcb6b3b2fdd7e64d74d04382 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 30 Sep 2019 16:05:39 -0400 Subject: [PATCH 3/9] Add the Security::ProjectsController This controller will be responsible for the addition, removal, and listing of the projects included in the dashboard. --- .../security/projects_controller.rb | 29 ++++++ ee/config/routes/security.rb | 2 +- .../security/dashboard_controller_spec.rb | 2 - .../security/projects_controller_spec.rb | 97 +++++++++++++++++++ 4 files changed, 127 insertions(+), 3 deletions(-) create mode 100644 ee/app/controllers/security/projects_controller.rb create mode 100644 ee/spec/controllers/security/projects_controller_spec.rb diff --git a/ee/app/controllers/security/projects_controller.rb b/ee/app/controllers/security/projects_controller.rb new file mode 100644 index 00000000000000..2994700a901805 --- /dev/null +++ b/ee/app/controllers/security/projects_controller.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Security + class ProjectsController < ApplicationController + before_action :authorize_read_security_dashboard! + before_action do + push_frontend_feature_flag(:security_dashboard) + end + + def index + head :ok + end + + def create + head :ok + end + + def destroy + head :ok + end + + private + + def authorize_read_security_dashboard! + render_404 unless Feature.enabled?(:security_dashboard) && + can?(current_user, :read_security_dashboard) + end + end +end diff --git a/ee/config/routes/security.rb b/ee/config/routes/security.rb index 8c38c793bc1f49..af565a20ccb939 100644 --- a/ee/config/routes/security.rb +++ b/ee/config/routes/security.rb @@ -3,5 +3,5 @@ namespace :security do root to: 'dashboard#show' - resources :projects, only: [:index, :create, :delete] + resources :projects, only: [:index, :create, :destroy] end diff --git a/ee/spec/controllers/security/dashboard_controller_spec.rb b/ee/spec/controllers/security/dashboard_controller_spec.rb index e8ed3358348880..ee4cb8a5d4d70c 100644 --- a/ee/spec/controllers/security/dashboard_controller_spec.rb +++ b/ee/spec/controllers/security/dashboard_controller_spec.rb @@ -3,8 +3,6 @@ require 'spec_helper' describe Security::DashboardController do - include Rails.application.routes.url_helpers - let(:user) { create(:user) } before do diff --git a/ee/spec/controllers/security/projects_controller_spec.rb b/ee/spec/controllers/security/projects_controller_spec.rb new file mode 100644 index 00000000000000..c6025753a6942d --- /dev/null +++ b/ee/spec/controllers/security/projects_controller_spec.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Security::ProjectsController do + let(:user) { create(:user) } + + before do + stub_feature_flags(security_dashboard: true) + stub_licensed_features(security_dashboard: true) + sign_in(user) + end + + describe 'GET #index' do + it 'responds with success' do + get :index + + expect(response).to have_gitlab_http_status(:ok) + end + + context 'when the instance does not have an Ultimate license' do + it '404s' do + stub_licensed_features(security_dashboard: false) + + get :index + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when the security dashboard feature is disabled' do + it '404s' do + stub_feature_flags(security_dashboard: false) + + get :index + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + + describe 'POST #create' do + it 'responds with success' do + post :create + + expect(response).to have_gitlab_http_status(:ok) + end + + context 'when the instance does not have an Ultimate license' do + it '404s' do + stub_licensed_features(security_dashboard: false) + + post :create + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when the security dashboard feature is disabled' do + it '404s' do + stub_feature_flags(security_dashboard: false) + + post :create + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + + describe 'DELETE #destroy' do + it 'responds with success' do + delete :destroy, params: { id: 1 } + + expect(response).to have_gitlab_http_status(:ok) + end + + context 'when the instance does not have an Ultimate license' do + it '404s' do + stub_licensed_features(security_dashboard: false) + + delete :destroy, params: { id: 1 } + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when the security dashboard feature is disabled' do + it '404s' do + stub_feature_flags(security_dashboard: false) + + delete :destroy, params: { id: 1 } + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end +end -- GitLab From 4635c7809d8a094d86b543d3bedfdd094b441cfb Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 30 Sep 2019 16:12:19 -0400 Subject: [PATCH 4/9] Extract the Security::ApplicationController No need to duplicate the permissions logic --- .../security/application_controller.rb | 17 +++++++++++++++++ .../security/dashboard_controller.rb | 14 +------------- .../controllers/security/projects_controller.rb | 14 +------------- 3 files changed, 19 insertions(+), 26 deletions(-) create mode 100644 ee/app/controllers/security/application_controller.rb diff --git a/ee/app/controllers/security/application_controller.rb b/ee/app/controllers/security/application_controller.rb new file mode 100644 index 00000000000000..ebab1169bf15ad --- /dev/null +++ b/ee/app/controllers/security/application_controller.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Security + class ApplicationController < ::ApplicationController + before_action :authorize_read_security_dashboard! + before_action do + push_frontend_feature_flag(:security_dashboard) + end + + private + + def authorize_read_security_dashboard! + render_404 unless Feature.enabled?(:security_dashboard) && + can?(current_user, :read_security_dashboard) + end + end +end diff --git a/ee/app/controllers/security/dashboard_controller.rb b/ee/app/controllers/security/dashboard_controller.rb index ae8b037e391b67..0c3bbbd0772079 100644 --- a/ee/app/controllers/security/dashboard_controller.rb +++ b/ee/app/controllers/security/dashboard_controller.rb @@ -1,21 +1,9 @@ # frozen_string_literal: true module Security - class DashboardController < ApplicationController - before_action :authorize_read_security_dashboard! - before_action do - push_frontend_feature_flag(:security_dashboard) - end - + class DashboardController < ::Security::ApplicationController def show head :ok end - - private - - def authorize_read_security_dashboard! - render_404 unless Feature.enabled?(:security_dashboard) && - can?(current_user, :read_security_dashboard) - end end end diff --git a/ee/app/controllers/security/projects_controller.rb b/ee/app/controllers/security/projects_controller.rb index 2994700a901805..7147453a080aa4 100644 --- a/ee/app/controllers/security/projects_controller.rb +++ b/ee/app/controllers/security/projects_controller.rb @@ -1,12 +1,7 @@ # frozen_string_literal: true module Security - class ProjectsController < ApplicationController - before_action :authorize_read_security_dashboard! - before_action do - push_frontend_feature_flag(:security_dashboard) - end - + class ProjectsController < ::Security::ApplicationController def index head :ok end @@ -18,12 +13,5 @@ def create def destroy head :ok end - - private - - def authorize_read_security_dashboard! - render_404 unless Feature.enabled?(:security_dashboard) && - can?(current_user, :read_security_dashboard) - end end end -- GitLab From 3ccec1ca4f8548763bd3a79d9643a3a21b1c74f2 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 30 Sep 2019 17:24:51 -0400 Subject: [PATCH 5/9] Change security_path to security_root_path Fixed! --- ee/app/views/dashboard/_nav_link_list.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/views/dashboard/_nav_link_list.html.haml b/ee/app/views/dashboard/_nav_link_list.html.haml index f9bf02a9d2470a..135987ac8b6ca1 100644 --- a/ee/app/views/dashboard/_nav_link_list.html.haml +++ b/ee/app/views/dashboard/_nav_link_list.html.haml @@ -5,5 +5,5 @@ = link_to operations_path, class: 'dropdown-item' do = _('Operations') - if dashboard_nav_link?(:security) - = link_to security_path, class: 'dropdown-item' do + = link_to security_root_path, class: 'dropdown-item' do = _('Security') -- GitLab From d4be6b111f314fb9fa9283003e23900138992f31 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 7 Oct 2019 19:37:39 +0000 Subject: [PATCH 6/9] Apply suggestion to ee/spec/controllers/security/dashboard_controller_spec.rb --- ee/spec/controllers/security/dashboard_controller_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/ee/spec/controllers/security/dashboard_controller_spec.rb b/ee/spec/controllers/security/dashboard_controller_spec.rb index ee4cb8a5d4d70c..2e0eb2d9cd75c0 100644 --- a/ee/spec/controllers/security/dashboard_controller_spec.rb +++ b/ee/spec/controllers/security/dashboard_controller_spec.rb @@ -6,7 +6,6 @@ let(:user) { create(:user) } before do - stub_feature_flags(security_dashboard: true) stub_licensed_features(security_dashboard: true) sign_in(user) end -- GitLab From 51393cc6c3b6eb5ffcd4653bdb4a2e49342408d1 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 7 Oct 2019 19:37:52 +0000 Subject: [PATCH 7/9] Apply suggestion to ee/spec/controllers/security/projects_controller_spec.rb --- ee/spec/controllers/security/projects_controller_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/ee/spec/controllers/security/projects_controller_spec.rb b/ee/spec/controllers/security/projects_controller_spec.rb index c6025753a6942d..39458045f13c48 100644 --- a/ee/spec/controllers/security/projects_controller_spec.rb +++ b/ee/spec/controllers/security/projects_controller_spec.rb @@ -6,7 +6,6 @@ let(:user) { create(:user) } before do - stub_feature_flags(security_dashboard: true) stub_licensed_features(security_dashboard: true) sign_in(user) end -- GitLab From 7728ade35bf91c6a8875f02f6154f42ce101c20c Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Wed, 9 Oct 2019 11:49:03 -0400 Subject: [PATCH 8/9] Restrict access for anonymous users It's not like they'll have access to any projects anyway. --- ee/app/policies/ee/global_policy.rb | 2 +- ee/spec/policies/global_policy_spec.rb | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/ee/app/policies/ee/global_policy.rb b/ee/app/policies/ee/global_policy.rb index 07abf65046d52a..deaaeaab42e4a0 100644 --- a/ee/app/policies/ee/global_policy.rb +++ b/ee/app/policies/ee/global_policy.rb @@ -14,7 +14,7 @@ module GlobalPolicy end rule { operations_dashboard_available }.enable :read_operations_dashboard - rule { security_dashboard_available }.enable :read_security_dashboard + rule { ~anonymous & security_dashboard_available }.enable :read_security_dashboard rule { admin }.policy do enable :read_licenses diff --git a/ee/spec/policies/global_policy_spec.rb b/ee/spec/policies/global_policy_spec.rb index c847b3be50a0a9..09bf765cf42b40 100644 --- a/ee/spec/policies/global_policy_spec.rb +++ b/ee/spec/policies/global_policy_spec.rb @@ -62,7 +62,15 @@ stub_licensed_features(security_dashboard: true) end - it { is_expected.to be_allowed(:read_security_dashboard) } + context 'and the user is not logged in' do + let(:current_user) { nil } + + it { is_expected.not_to be_allowed(:read_security_dashboard) } + end + + context 'and the user is logged in' do + it { is_expected.to be_allowed(:read_security_dashboard) } + end end context 'when the instance does not have an Ultimate license' do -- GitLab From 542471a2f17169201b09faf16b71c9f5b05a4746 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Thu, 10 Oct 2019 18:08:35 -0400 Subject: [PATCH 9/9] Test anonymous is redirected Ensures that anonymous users are redirected to the sign in page. Also, extracts Security::ApplicationController shared examples. --- .../security/dashboard_controller_spec.rb | 31 +------- .../security/projects_controller_spec.rb | 79 ++----------------- .../application_controller_shared_examples.rb | 48 +++++++++++ 3 files changed, 56 insertions(+), 102 deletions(-) create mode 100644 ee/spec/support/shared_examples/controllers/security/application_controller_shared_examples.rb diff --git a/ee/spec/controllers/security/dashboard_controller_spec.rb b/ee/spec/controllers/security/dashboard_controller_spec.rb index 2e0eb2d9cd75c0..a4252a91e8504f 100644 --- a/ee/spec/controllers/security/dashboard_controller_spec.rb +++ b/ee/spec/controllers/security/dashboard_controller_spec.rb @@ -3,37 +3,10 @@ require 'spec_helper' describe Security::DashboardController do - let(:user) { create(:user) } - - before do - stub_licensed_features(security_dashboard: true) - sign_in(user) - end - describe 'GET #show' do - it 'responds with success' do - get :show - - expect(response).to have_gitlab_http_status(:ok) - end - - context 'when the instance does not have an Ultimate license' do - it '404s' do - stub_licensed_features(security_dashboard: false) - + it_behaves_like Security::ApplicationController do + let(:security_application_controller_child_action) do get :show - - expect(response).to have_gitlab_http_status(:not_found) - end - end - - context 'when the security dashboard feature is disabled' do - it '404s' do - stub_feature_flags(security_dashboard: false) - - get :show - - expect(response).to have_gitlab_http_status(:not_found) end end end diff --git a/ee/spec/controllers/security/projects_controller_spec.rb b/ee/spec/controllers/security/projects_controller_spec.rb index 39458045f13c48..ae4e915db3db9c 100644 --- a/ee/spec/controllers/security/projects_controller_spec.rb +++ b/ee/spec/controllers/security/projects_controller_spec.rb @@ -3,93 +3,26 @@ require 'spec_helper' describe Security::ProjectsController do - let(:user) { create(:user) } - - before do - stub_licensed_features(security_dashboard: true) - sign_in(user) - end - describe 'GET #index' do - it 'responds with success' do - get :index - - expect(response).to have_gitlab_http_status(:ok) - end - - context 'when the instance does not have an Ultimate license' do - it '404s' do - stub_licensed_features(security_dashboard: false) - - get :index - - expect(response).to have_gitlab_http_status(:not_found) - end - end - - context 'when the security dashboard feature is disabled' do - it '404s' do - stub_feature_flags(security_dashboard: false) - + it_behaves_like Security::ApplicationController do + let(:security_application_controller_child_action) do get :index - - expect(response).to have_gitlab_http_status(:not_found) end end end describe 'POST #create' do - it 'responds with success' do - post :create - - expect(response).to have_gitlab_http_status(:ok) - end - - context 'when the instance does not have an Ultimate license' do - it '404s' do - stub_licensed_features(security_dashboard: false) - + it_behaves_like Security::ApplicationController do + let(:security_application_controller_child_action) do post :create - - expect(response).to have_gitlab_http_status(:not_found) - end - end - - context 'when the security dashboard feature is disabled' do - it '404s' do - stub_feature_flags(security_dashboard: false) - - post :create - - expect(response).to have_gitlab_http_status(:not_found) end end end describe 'DELETE #destroy' do - it 'responds with success' do - delete :destroy, params: { id: 1 } - - expect(response).to have_gitlab_http_status(:ok) - end - - context 'when the instance does not have an Ultimate license' do - it '404s' do - stub_licensed_features(security_dashboard: false) - + it_behaves_like Security::ApplicationController do + let(:security_application_controller_child_action) do delete :destroy, params: { id: 1 } - - expect(response).to have_gitlab_http_status(:not_found) - end - end - - context 'when the security dashboard feature is disabled' do - it '404s' do - stub_feature_flags(security_dashboard: false) - - delete :destroy, params: { id: 1 } - - expect(response).to have_gitlab_http_status(:not_found) end end end diff --git a/ee/spec/support/shared_examples/controllers/security/application_controller_shared_examples.rb b/ee/spec/support/shared_examples/controllers/security/application_controller_shared_examples.rb new file mode 100644 index 00000000000000..1a88d9086abcec --- /dev/null +++ b/ee/spec/support/shared_examples/controllers/security/application_controller_shared_examples.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'spec_helper' + +shared_examples Security::ApplicationController do + context 'when the user is authenticated' do + let(:security_application_controller_user) { create(:user) } + + before do + stub_licensed_features(security_dashboard: true) + sign_in(security_application_controller_user) + end + + it 'responds with success' do + security_application_controller_child_action + + expect(response).to have_gitlab_http_status(:ok) + end + + context 'and the instance does not have an Ultimate license' do + it '404s' do + stub_licensed_features(security_dashboard: false) + + security_application_controller_child_action + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'and the security dashboard feature is disabled' do + it '404s' do + stub_feature_flags(security_dashboard: false) + + security_application_controller_child_action + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + + context 'when the user is not authenticated' do + it 'redirects the user to the sign in page' do + security_application_controller_child_action + + expect(response).to redirect_to(new_user_session_path) + end + end +end -- GitLab