Merge branch 'design-management-permissions' into 'master'

Added permissions check to design management

See merge request gitlab-org/gitlab-ee!10501

app/graphql/types/permission_types/issue.rb

@@ -12,3 +12,5 @@ module Types
     end
   end
 end
+
+Types::PermissionTypes::Issue.prepend(::EE::Types::PermissionTypes::Issue)

app/graphql/types/permission_types/project.rb

@@ -20,3 +20,5 @@ module Types
     end
   end
 end
+
+Types::PermissionTypes::Project.prepend(EE::Types::PermissionTypes::Project)

app/policies/issue_policy.rb

@@ -25,3 +25,5 @@ class IssuePolicy < IssuablePolicy
     prevent :reopen_issue
   end
 end
+
+IssuePolicy.prepend(::EE::IssuePolicy)

ee/app/assets/javascripts/design_management/components/upload/form.vue

@@ -11,6 +11,10 @@ export default {
       type: Boolean,
       required: true,
     },
+    canUploadDesign: {
+      type: Boolean,
+      required: true,
+    },
   },
   methods: {
     openFileUpload() {
@@ -27,6 +31,7 @@ export default {
   <header class="row-content-block border-top-0 p-2 d-flex">
     <div>
       <gl-button
+        v-if="canUploadDesign"
         :disabled="isSaving"
         variant="primary"
         class="btn-inverted"

ee/app/assets/javascripts/design_management/index.js

@@ -5,6 +5,9 @@ import App from './components/app.vue';
 import apolloProvider from './graphql';
 
 export default () => {
+  const el = document.getElementById('js-design-management');
+  const { issueIid, projectPath } = el.dataset;
+
   $('.js-issue-tabs').on('shown.bs.tab', ({ target: { id } }) => {
     if (id === 'designs' && router.currentRoute.name === 'root') {
       router.push('/designs');
@@ -13,8 +16,15 @@ export default () => {
     }
   });
 
+  apolloProvider.clients.defaultClient.cache.writeData({
+    data: {
+      projectPath,
+      issueIid,
+    },
+  });
+
   return new Vue({
-    el: document.getElementById('js-design-management'),
+    el,
     router,
     apolloProvider,
     render(createElement) {

ee/app/assets/javascripts/design_management/pages/index.vue

@@ -6,6 +6,8 @@ import DesignList from '../components/list/index.vue';
 import UploadForm from '../components/upload/form.vue';
 import allDesignsQuery from '../queries/allDesigns.graphql';
 import uploadDesignQuery from '../queries/uploadDesign.graphql';
+import appDataQuery from '../queries/appData.graphql';
+import permissionsQuery from '../queries/permissions.graphql';
 
 export default {
   components: {
@@ -14,27 +16,53 @@ export default {
     UploadForm,
   },
   apollo: {
+    appData: {
+      query: appDataQuery,
+      manual: true,
+      result({ data: { projectPath, issueIid } }) {
+        this.projectPath = projectPath;
+        this.issueIid = issueIid;
+      },
+    },
     designs: {
       query: allDesignsQuery,
       error() {
         this.error = true;
       },
     },
+    permissions: {
+      query: permissionsQuery,
+      variables() {
+        return {
+          fullPath: this.projectPath,
+          iid: this.issueIid,
+        };
+      },
+      update: data => data.project.issue.userPermissions,
+    },
   },
   data() {
     return {
       designs: [],
+      permissions: {},
       error: false,
       isSaving: false,
+      projectPath: '',
+      issueIid: null,
     };
   },
   computed: {
     isLoading() {
-      return this.$apollo.queries.designs.loading;
+      return this.$apollo.queries.designs.loading && this.$apollo.queries.permissions.loading;
+    },
+    canCreateDesign() {
+      return this.permissions.createDesign;
     },
   },
   methods: {
     onUploadDesign(files) {
+      if (!this.canCreateDesign) return null;
+
       const optimisticResponse = [...files].map(file => ({
         __typename: 'Design',
         id: -1,
@@ -80,7 +108,12 @@ export default {
 
 <template>
   <div>
-    <upload-form :is-saving="isSaving" @upload="onUploadDesign" />
+    <upload-form
+      v-if="canCreateDesign"
+      :can-upload-design="canCreateDesign"
+      :is-saving="isSaving"
+      @upload="onUploadDesign"
+    />
     <div class="mt-4">
       <gl-loading-icon v-if="isLoading" size="md" />
       <div v-else-if="error" class="alert alert-danger">

ee/app/assets/javascripts/design_management/queries/appData.graphql

+query projectFullPath {
+  projectPath @client
+  issueIid @client
+}

ee/app/assets/javascripts/design_management/queries/permissions.graphql

+query permissions($fullPath: ID!, $iid: ID!) {
+  project(fullPath: $fullPath) {
+    issue(iid: $iid) {
+      userPermissions {
+        createDesign
+      }
+    }
+  }
+}

ee/app/assets/javascripts/pages/projects/issues/show/index.js

@@ -8,7 +8,7 @@ document.addEventListener('DOMContentLoaded', () => {
   initSidebarBundle();
   initRelatedIssues();
 
-  if (gon.features.versionedDesigns) {
+  if (document.getElementById('js-design-management')) {
     import(/* webpackChunkName: 'design_management' */ 'ee/design_management')
       .then(module => module.default())
       .catch(() => {});

ee/app/controllers/ee/projects/issues_controller.rb

@@ -16,9 +16,6 @@ module EE
         before_action :check_export_issues_available!, only: [:export_csv]
         before_action :check_service_desk_available!, only: [:service_desk]
         before_action :whitelist_query_limiting_ee, only: [:update]
-        before_action only: :show do
-          push_frontend_feature_flag(:versioned_designs)
-        end
       end
 
       override :issue_except_actions

ee/app/graphql/ee/types/permission_types/issue.rb

+# frozen_string_literal: true
+
+module EE
+  module Types
+    module PermissionTypes
+      module Issue
+        extend ActiveSupport::Concern
+
+        prepended do
+          abilities :read_design, :create_design, :destroy_design
+        end
+      end
+    end
+  end
+end

ee/app/graphql/ee/types/permission_types/project.rb

+# frozen_string_literal: true
+
+module EE
+  module Types
+    module PermissionTypes
+      module Project
+        extend ActiveSupport::Concern
+
+        prepended do
+          abilities :read_design, :create_design, :destroy_design
+        end
+      end
+    end
+  end
+end

ee/app/models/ee/project.rb

@@ -575,6 +575,13 @@ module EE
       super.presence || build_feature_usage
     end
 
+    def design_management_enabled?
+      # Checking both feature availability on the license, as well as the feature
+      # flag, because we don't want to enable design_management by default on
+      # on prem installs yet.
+      feature_available?(:design_management) && ::Feature.enabled?(:design_management, self)
+    end
+
     private
 
     def set_override_pull_mirror_available

ee/app/models/license.rb

@@ -75,6 +75,7 @@ class License < ApplicationRecord
     batch_comments
     issues_analytics
     merge_pipelines
+    design_management
   ]
   EEP_FEATURES.freeze
 

ee/app/policies/design_management/design_policy.rb

+# frozen_string_literal: true
+
+module DesignManagement
+  class DesignPolicy < ::BasePolicy
+    # The IssuePolicy will delegate to the ProjectPolicy
+    delegate { @subject.issue }
+  end
+end

ee/app/policies/ee/issue_policy.rb

+# frozen_string_literal: true
+
+module EE
+  module IssuePolicy
+    extend ActiveSupport::Concern
+    prepended do
+      rule { ~can?(:read_issue) }.policy do
+        prevent :read_design
+        prevent :create_design
+        prevent :destroy_design
+      end
+    end
+  end
+end

ee/app/policies/ee/project_policy.rb

@@ -11,6 +11,7 @@ module EE
       vulnerability_feedback
       license_management
       feature_flag
+      design
     ].freeze
 
     prepended do
@@ -75,8 +76,17 @@ module EE
         !@subject.feature_available?(:feature_flags)
       end
 
+      with_scope :subject
+      condition(:design_management_disabled) do
+        !@subject.design_management_enabled?
+      end
+
       rule { admin }.enable :change_repository_storage
 
+      rule { can?(:public_access) }.policy do
+        enable :read_design
+      end
+
       rule { support_bot }.enable :guest_access
       rule { support_bot & ~service_desk_enabled }.policy do
         prevent :create_note
@@ -114,6 +124,8 @@ module EE
         enable :update_feature_flag
         enable :destroy_feature_flag
         enable :admin_feature_flag
+        enable :create_design
+        enable :destroy_design
       end
 
       rule { can?(:public_access) }.enable :read_package
@@ -209,6 +221,14 @@ module EE
       end
 
       rule { web_ide_terminal_available & can?(:create_pipeline) & can?(:maintainer_access) }.enable :create_web_ide_terminal
+
+      # Design abilities could also be prevented in the issue policy.
+      # If the user cannot read the issue, then they cannot see the designs.
+      rule { design_management_disabled }.policy do
+        prevent :read_design
+        prevent :create_design
+        prevent :destroy_design
+      end
     end
   end
 end

ee/app/views/projects/issues/_discussion.html.haml

-- if Feature.enabled?(:versioned_designs)
+- if can?(current_user, :read_design, @issue)
   %ul.nav-tabs.nav.nav-links{ role: 'tablist' }
     %li
       = link_to '#discussion-tab', class: 'active js-issue-tabs', id: 'discussion', role: 'tab', 'aria-controls': 'js-discussion', 'aria-selected': 'true', data: { toggle: 'tab', target: '#discussion-tab' } do
@@ -12,6 +12,6 @@
     #discussion-tab.tab-pane.show.active{ role: 'tabpanel', 'aria-labelledby': 'discussion' }
       = render_ce 'projects/issues/discussion'
     #designs-tab.tab-pane{ role: 'tabpanel', 'aria-labelledby': 'designs' }
-      #js-design-management
+      #js-design-management{ data: { project_path: @project.full_path, issue_iid: @issue.iid } }
 - else
   = render_ce 'projects/issues/discussion'

ee/spec/features/projects/issues/design_management/user_permissions_upload_spec.rb

+require 'spec_helper'
+
+describe 'User design permissions', :js do
+  let(:project) { create(:project_empty_repo, :public) }
+  let(:issue) { create(:issue, project: project) }
+
+  before do
+    stub_licensed_features(design_management: true)
+
+    visit project_issue_path(project, issue)
+
+    click_link 'Designs'
+
+    wait_for_requests
+  end
+
+  it 'user does not have permissions to upload design' do
+    expect(page).not_to have_field('design_file')
+  end
+end

ee/spec/features/projects/issues/design_management/user_uploads_designs_spec.rb

 require 'spec_helper'
 
 describe 'User uploads new design', :js do
+  let(:user) { project.owner }
   let(:project) { create(:project_empty_repo, :public) }
   let(:issue) { create(:issue, project: project) }
 
   before do
-    visit project_issue_path(project, issue)
+    sign_in(user)
+  end
+
+  context "when the feature is available" do
+    before do
+      stub_licensed_features(design_management: true)
+
+      visit project_issue_path(project, issue)
 
-    click_link 'Designs'
+      click_link 'Designs'
+
+      wait_for_requests
+    end
 
-    wait_for_requests
+    it 'uploads design' do
+      attach_file(:design_file, logo_fixture, make_visible: true)
+
+      expect(page).to have_selector('.js-design-list-item', count: 6)
+
+      within first('#designs-tab .card') do
+        expect(page).to have_content('dk.png')
+        expect(page).to have_content('Updated just now')
+      end
+    end
   end
 
-  it 'uploads design' do
-    attach_file(:design_file, logo_fixture, make_visible: true)
+  context 'when the feature is not available' do
+    before do
+      stub_licensed_features(design_management: false)
 
-    expect(page).to have_selector('.js-design-list-item', count: 6)
+      visit project_issue_path(project, issue)
+    end
 
-    within first('#designs-tab .card') do
-      expect(page).to have_content('dk.png')
-      expect(page).to have_content('Updated just now')
+    it 'does not show the designs link' do
+      expect(page).not_to have_link('Designs')
     end
   end
 

ee/spec/features/projects/issues/design_management/user_views_design_spec.rb

@@ -5,6 +5,8 @@ describe 'User views issue designs', :js do
   let(:issue) { create(:issue, project: project) }
 
   before do
+    stub_licensed_features(design_management: true)
+
     visit project_issue_path(project, issue)
 
     click_link 'Designs'

ee/spec/features/projects/issues/design_management/user_views_designs_spec.rb

@@ -5,6 +5,8 @@ describe 'User views issue designs', :js do
   let(:issue) { create(:issue, project: project) }
 
   before do
+    stub_licensed_features(design_management: true)
+
     visit project_issue_path(project, issue)
 
     click_link 'Designs'

ee/spec/frontend/design_management/components/upload/__snapshots__/form_spec.js.snap

 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`Design management upload form component hides button if cant upload 1`] = `
+<header
+  class="row-content-block border-top-0 p-2 d-flex"
+>
+  <div>
+    <!---->
+     
+    <input
+      accept="image/*"
+      class="hide"
+      multiple="multiple"
+      name="design_file"
+      type="file"
+    />
+  </div>
+</header>
+`;
+
 exports[`Design management upload form component renders loading icon 1`] = `
 <header
   class="row-content-block border-top-0 p-2 d-flex"

ee/spec/frontend/design_management/components/upload/form_spec.js

@@ -4,10 +4,11 @@ import UploadForm from 'ee/design_management/components/upload/form.vue';
 describe('Design management upload form component', () => {
   let vm;
 
-  function createComponent(isSaving = false) {
+  function createComponent(isSaving = false, canUploadDesign = true) {
     vm = shallowMount(UploadForm, {
       propsData: {
         isSaving,
+        canUploadDesign,
       },
     });
   }
@@ -24,6 +25,12 @@ describe('Design management upload form component', () => {
     expect(vm.element).toMatchSnapshot();
   });
 
+  it('hides button if cant upload', () => {
+    createComponent(false, false);
+
+    expect(vm.element).toMatchSnapshot();
+  });
+
   describe('onFileUploadChange', () => {
     it('emits upload event', () => {
       createComponent();

ee/spec/frontend/design_management/pages/__snapshots__/index_spec.js.snap

@@ -2,7 +2,9 @@
 
 exports[`Design management index page designs renders designs list 1`] = `
 <div>
-  <uploadform-stub />
+  <uploadform-stub
+    canuploaddesign="true"
+  />
    
   <div
     class="mt-4"
@@ -18,7 +20,9 @@ exports[`Design management index page designs renders designs list 1`] = `
 
 exports[`Design management index page designs renders empty text 1`] = `
 <div>
-  <uploadform-stub />
+  <uploadform-stub
+    canuploaddesign="true"
+  />
    
   <div
     class="mt-4"
@@ -34,7 +38,9 @@ exports[`Design management index page designs renders empty text 1`] = `
 
 exports[`Design management index page designs renders error 1`] = `
 <div>
-  <uploadform-stub />
+  <uploadform-stub
+    canuploaddesign="true"
+  />
    
   <div
     class="mt-4"
@@ -54,7 +60,9 @@ exports[`Design management index page designs renders error 1`] = `
 
 exports[`Design management index page designs renders loading icon 1`] = `
 <div>
-  <uploadform-stub />
+  <uploadform-stub
+    canuploaddesign="true"
+  />
    
   <div
     class="mt-4"

ee/spec/frontend/design_management/pages/index_spec.js

@@ -3,15 +3,19 @@ import Index from 'ee/design_management/pages/index.vue';
 import uploadDesignQuery from 'ee/design_management/queries/uploadDesign.graphql';
 
 describe('Design management index page', () => {
-  const mutate = jest.fn(() => Promise.resolve());
+  let mutate;
   let vm;
 
   function createComponent(loading = false) {
+    mutate = jest.fn(() => Promise.resolve());
     const $apollo = {
       queries: {
         designs: {
           loading,
         },
+        permissions: {
+          loading,
+        },
       },
       mutate,
     };
@@ -20,6 +24,12 @@ describe('Design management index page', () => {
       mocks: { $apollo },
       stubs: ['router-view'],
     });
+
+    vm.setData({
+      permissions: {
+        createDesign: true,
+      },
+    });
   }
 
   describe('designs', () => {
@@ -86,6 +96,20 @@ describe('Design management index page', () => {
         });
     });
 
+    it('does not call apollo mutate if createDesign is false', () => {
+      createComponent();
+
+      vm.setData({
+        permissions: {
+          createDesign: false,
+        },
+      });
+
+      vm.vm.onUploadDesign([]);
+
+      expect(mutate).not.toHaveBeenCalled();
+    });
+
     it('sets isSaving', () => {
       createComponent();
 

ee/spec/frontend/design_management/router_spec.js

@@ -24,7 +24,11 @@ describe('Design management router', () => {
       router,
       mocks: {
         $apollo: {
-          queries: { designs: { loading: true }, design: { loading: true } },
+          queries: {
+            designs: { loading: true },
+            design: { loading: true },
+            permissions: { loading: true },
+          },
         },
       },
     });

ee/spec/graphql/types/permission_types/issue_spec.rb

+require 'spec_helper'
+
+describe Types::PermissionTypes::Issue do
+  it "exposes design permissions" do
+    expected_permissions = [
+      :read_design, :create_design, :destroy_design
+    ]
+
+    expected_permissions.each do |permission|
+      expect(described_class).to have_graphql_field(permission)
+    end
+  end
+end

ee/spec/graphql/types/permission_types/project_spec.rb

+require 'spec_helper'
+
+describe Types::PermissionTypes::Project do
+  it "exposes design permissions" do
+    expected_permissions = [
+      :read_design, :create_design, :destroy_design
+    ]
+
+    expected_permissions.each do |permission|
+      expect(described_class).to have_graphql_field(permission)
+    end
+  end
+end

ee/spec/models/project_spec.rb

@@ -1767,6 +1767,27 @@ describe Project do
     end
   end
 
+  describe "#design_management_enabled?" do
+    let(:project) { build(:project) }
+    where(:feature_enabled, :license_enabled, :expected) do
+      false | false | false
+      false | true  | false
+      true  | false | false
+      true  | true  | true
+    end
+
+    with_them do
+      before do
+        stub_licensed_features(design_management: license_enabled)
+        stub_feature_flags(design_management: feature_enabled)
+      end
+
+      it "knows if design management is available" do
+        expect(project.design_management_enabled?).to be(expected)
+      end
+    end
+  end
+
   # Despite stubbing the current node as the primary or secondary, the
   # behaviour for EE::Project#lfs_http_url_to_repo() is to call
   # Project#lfs_http_url_to_repo() which does not have a Geo context.

ee/spec/policies/design_management/design_policy_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe DesignManagement::DesignPolicy do
+  include_context 'ProjectPolicy context'
+
+  let(:guest_design_abilities) { %i[read_design] }
+  let(:developer_design_abilities) do
+    %i[create_design destroy_design]
+  end
+  let(:design_abilities) { guest_design_abilities + developer_design_abilities }
+
+  let(:issue) { create(:issue, project: project) }
+  let(:design) { create(:design, issue: issue) }
+
+  subject(:design_policy) { described_class.new(current_user, design) }
+
+  shared_examples_for "design abilities not available" do
+    context "for owners" do
+      let(:current_user) { owner }
+
+      it { is_expected.to be_disallowed(*design_abilities) }
+    end
+
+    context "for admins" do
+      let(:current_user) { admin }
+
+      it { is_expected.to be_disallowed(*design_abilities) }
+    end
+
+    context "for maintainers" do
+      let(:current_user) { maintainer }
+
+      it { is_expected.to be_disallowed(*design_abilities) }
+    end
+
+    context "for developers" do
+      let(:current_user) { developer }
+
+      it { is_expected.to be_disallowed(*design_abilities) }
+    end
+
+    context "for reporters" do
+      let(:current_user) { reporter }
+
+      it { is_expected.to be_disallowed(*design_abilities) }
+    end
+
+    context "for guests" do
+      let(:current_user) { guest }
+
+      it { is_expected.to be_disallowed(*design_abilities) }
+    end
+
+    context "for anonymous users" do
+      let(:current_user) { nil }
+
+      it { is_expected.to be_disallowed(*design_abilities) }
+    end
+  end
+
+  shared_examples_for "design abilities available for members" do
+    context "for owners" do
+      let(:current_user) { owner }
+
+      it { is_expected.to be_allowed(*design_abilities) }
+    end
+
+    context "for admins" do
+      let(:current_user) { admin }
+
+      it { is_expected.to be_allowed(*design_abilities) }
+    end
+
+    context "for maintainers" do
+      let(:current_user) { maintainer }
+
+      it { is_expected.to be_allowed(*design_abilities) }
+    end
+
+    context "for developers" do
+      let(:current_user) { developer }
+
+      it { is_expected.to be_allowed(*design_abilities) }
+    end
+
+    context "for reporters" do
+      let(:current_user) { reporter }
+
+      it { is_expected.to be_allowed(*guest_design_abilities) }
+      it { is_expected.to be_disallowed(*developer_design_abilities) }
+    end
+  end
+
+  context "when the feature flag is off" do
+    before do
+      stub_licensed_features(design_management: true)
+      stub_feature_flags(design_management: false)
+    end
+
+    it_behaves_like "design abilities not available"
+  end
+
+  context "when the license does not include the feature" do
+    before do
+      stub_licensed_features(design_management: false)
+      stub_feature_flags(design_management: true)
+    end
+
+    it_behaves_like "design abilities not available"
+  end
+
+  context "when the feature is available" do
+    before do
+      stub_licensed_features(design_management: true)
+      stub_feature_flags(design_management: true)
+    end
+
+    it_behaves_like "design abilities available for members"
+
+    context "for guests" do
+      let(:current_user) { guest }
+
+      it { is_expected.to be_allowed(*guest_design_abilities) }
+      it { is_expected.to be_disallowed(*developer_design_abilities) }
+    end
+
+    context "for anonymous users" do
+      let(:current_user) { nil }
+
+      it { is_expected.to be_allowed(*guest_design_abilities) }
+      it { is_expected.to be_disallowed(*developer_design_abilities) }
+    end
+
+    context "when the issue is confidential" do
+      let(:issue) { create(:issue, :confidential, project: project) }
+
+      it_behaves_like "design abilities available for members"
+
+      context "for guests" do
+        let(:current_user) { guest }
+
+        it { is_expected.to be_disallowed(*design_abilities) }
+      end
+
+      context "for anonymous users" do
+        let(:current_user) { nil }
+
+        it { is_expected.to be_disallowed(*design_abilities) }
+      end
+    end
+
+    context "when the project is archived" do
+      let(:current_user) { owner }
+
+      before do
+        project.update!(archived: true)
+      end
+
+      it "only allows reading designs" do
+        expect(design_policy).to be_allowed(:read_design)
+        expect(design_policy).to be_disallowed(:create_design, :destroy_design)
+      end
+    end
+  end
+end

spec/graphql/types/permission_types/issue_spec.rb

@@ -7,6 +7,8 @@ describe Types::PermissionTypes::Issue do
       :create_note, :reopen_issue
     ]
 
-    expect(described_class).to have_graphql_fields(expected_permissions)
+    expected_permissions.each do |permission|
+      expect(described_class).to have_graphql_field(permission)
+    end
   end
 end

spec/graphql/types/permission_types/project_spec.rb

@@ -13,6 +13,8 @@ describe Types::PermissionTypes::Project do
       :update_wiki, :destroy_wiki, :create_pages, :destroy_pages, :read_pages_content
     ]
 
-    expect(described_class).to have_graphql_fields(expected_permissions)
+    expected_permissions.each do |permission|
+      expect(described_class).to have_graphql_field(permission)
+    end
   end
 end