From bcb0cb527d81fdfa0c67d9dbe4a7895394486f16 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Fri, 12 Feb 2021 15:29:00 -0600 Subject: [PATCH] Update IDE to alert when gpg commits required - Also renamed graphql query since we're fetching more than just userPermissions now. --- .../ide/components/commit_sidebar/form.vue | 18 ++--- app/assets/javascripts/ide/components/ide.vue | 13 ++-- app/assets/javascripts/ide/constants.js | 1 + app/assets/javascripts/ide/messages.js | 17 +++++ .../queries/getUserPermissions.query.graphql | 9 --- .../ide/queries/get_ide_project.query.graphql | 7 ++ .../ide/queries/ide_project.fragment.graphql | 7 ++ app/assets/javascripts/ide/services/index.js | 4 +- app/assets/javascripts/ide/stores/getters.js | 44 +++++++++++- ...lert-user-when-reject-unsigned-commits.yml | 5 ++ .../ide/queries/get_ide_project.query.graphql | 10 +++ ee/spec/features/ide/user_opens_ide_spec.rb | 40 +++++++++++ locale/gitlab.pot | 6 ++ .../components/commit_sidebar/form_spec.js | 5 +- spec/frontend/ide/components/ide_spec.js | 3 +- spec/frontend/ide/services/index_spec.js | 4 +- spec/frontend/ide/stores/getters_spec.js | 70 +++++++++++++++---- 17 files changed, 210 insertions(+), 53 deletions(-) create mode 100644 app/assets/javascripts/ide/messages.js delete mode 100644 app/assets/javascripts/ide/queries/getUserPermissions.query.graphql create mode 100644 app/assets/javascripts/ide/queries/get_ide_project.query.graphql create mode 100644 app/assets/javascripts/ide/queries/ide_project.fragment.graphql create mode 100644 changelogs/unreleased/213581-ide-alert-user-when-reject-unsigned-commits.yml create mode 100644 ee/app/assets/javascripts/ide/queries/get_ide_project.query.graphql create mode 100644 ee/spec/features/ide/user_opens_ide_spec.rb diff --git a/app/assets/javascripts/ide/components/commit_sidebar/form.vue b/app/assets/javascripts/ide/components/commit_sidebar/form.vue index cdb3eaff207382..2897f4cbf777bd 100644 --- a/app/assets/javascripts/ide/components/commit_sidebar/form.vue +++ b/app/assets/javascripts/ide/components/commit_sidebar/form.vue @@ -1,17 +1,13 @@ diff --git a/app/assets/javascripts/ide/components/ide.vue b/app/assets/javascripts/ide/components/ide.vue index 2816f89d60daf3..ff2644704d95fc 100644 --- a/app/assets/javascripts/ide/components/ide.vue +++ b/app/assets/javascripts/ide/components/ide.vue @@ -1,7 +1,7 @@ @@ -125,8 +120,8 @@ export default { class="ide position-relative d-flex flex-column align-items-stretch" :class="{ [`theme-${themeName}`]: themeName }" > - {{ - $options.MSG_CANNOT_PUSH_CODE + {{ + canPushCodeStatus.message }}
diff --git a/app/assets/javascripts/ide/constants.js b/app/assets/javascripts/ide/constants.js index ed6b750480b2aa..056df3739eec91 100644 --- a/app/assets/javascripts/ide/constants.js +++ b/app/assets/javascripts/ide/constants.js @@ -15,6 +15,7 @@ export const FILE_VIEW_MODE_PREVIEW = 'preview'; export const PERMISSION_CREATE_MR = 'createMergeRequestIn'; export const PERMISSION_READ_MR = 'readMergeRequest'; export const PERMISSION_PUSH_CODE = 'pushCode'; +export const PUSH_RULE_REJECT_UNSIGNED_COMMITS = 'rejectUnsignedCommits'; // The default permission object to use when the project data isn't available yet. // This helps us encapsulate checks like `canPushCode` without requiring an diff --git a/app/assets/javascripts/ide/messages.js b/app/assets/javascripts/ide/messages.js new file mode 100644 index 00000000000000..4298d4c627c918 --- /dev/null +++ b/app/assets/javascripts/ide/messages.js @@ -0,0 +1,17 @@ +import { s__ } from '~/locale'; + +export const MSG_CANNOT_PUSH_CODE = s__( + 'WebIDE|You need permission to edit files directly in this project. Fork this project to make your changes and submit a merge request.', +); + +export const MSG_CANNOT_PUSH_CODE_SHORT = s__( + 'WebIDE|You need permission to edit files directly in this project.', +); + +export const MSG_CANNOT_PUSH_UNSIGNED = s__( + 'WebIDE|This project does not accept unsigned commits. You will not be able to commit your changes through the Web IDE.', +); + +export const MSG_CANNOT_PUSH_UNSIGNED_SHORT = s__( + 'WebIDE|This project does not accept unsigned commits.', +); diff --git a/app/assets/javascripts/ide/queries/getUserPermissions.query.graphql b/app/assets/javascripts/ide/queries/getUserPermissions.query.graphql deleted file mode 100644 index f0b50793226ef2..00000000000000 --- a/app/assets/javascripts/ide/queries/getUserPermissions.query.graphql +++ /dev/null @@ -1,9 +0,0 @@ -query getUserPermissions($projectPath: ID!) { - project(fullPath: $projectPath) { - userPermissions { - createMergeRequestIn - readMergeRequest - pushCode - } - } -} diff --git a/app/assets/javascripts/ide/queries/get_ide_project.query.graphql b/app/assets/javascripts/ide/queries/get_ide_project.query.graphql new file mode 100644 index 00000000000000..6ceb36909f3d4b --- /dev/null +++ b/app/assets/javascripts/ide/queries/get_ide_project.query.graphql @@ -0,0 +1,7 @@ +#import "~/ide/queries/ide_project.fragment.graphql" + +query getIdeProject($projectPath: ID!) { + project(fullPath: $projectPath) { + ...IdeProject + } +} diff --git a/app/assets/javascripts/ide/queries/ide_project.fragment.graphql b/app/assets/javascripts/ide/queries/ide_project.fragment.graphql new file mode 100644 index 00000000000000..c107f2376f9165 --- /dev/null +++ b/app/assets/javascripts/ide/queries/ide_project.fragment.graphql @@ -0,0 +1,7 @@ +fragment IdeProject on Project { + userPermissions { + createMergeRequestIn + readMergeRequest + pushCode + } +} diff --git a/app/assets/javascripts/ide/services/index.js b/app/assets/javascripts/ide/services/index.js index e98653aedc2f4c..0aa08323d13ba9 100644 --- a/app/assets/javascripts/ide/services/index.js +++ b/app/assets/javascripts/ide/services/index.js @@ -1,14 +1,14 @@ +import getIdeProject from 'ee_else_ce/ide/queries/get_ide_project.query.graphql'; import Api from '~/api'; import axios from '~/lib/utils/axios_utils'; import { joinPaths, escapeFileUrl } from '~/lib/utils/url_utility'; -import getUserPermissions from '../queries/getUserPermissions.query.graphql'; import { query } from './gql'; const fetchApiProjectData = (projectPath) => Api.project(projectPath).then(({ data }) => data); const fetchGqlProjectData = (projectPath) => query({ - query: getUserPermissions, + query: getIdeProject, variables: { projectPath }, }).then(({ data }) => data.project); diff --git a/app/assets/javascripts/ide/stores/getters.js b/app/assets/javascripts/ide/stores/getters.js index 9b93fc74f761fd..a5bb32ec44a02e 100644 --- a/app/assets/javascripts/ide/stores/getters.js +++ b/app/assets/javascripts/ide/stores/getters.js @@ -7,7 +7,14 @@ import { PERMISSION_READ_MR, PERMISSION_CREATE_MR, PERMISSION_PUSH_CODE, + PUSH_RULE_REJECT_UNSIGNED_COMMITS, } from '../constants'; +import { + MSG_CANNOT_PUSH_CODE, + MSG_CANNOT_PUSH_CODE_SHORT, + MSG_CANNOT_PUSH_UNSIGNED, + MSG_CANNOT_PUSH_UNSIGNED_SHORT, +} from '../messages'; import { getChangesCountForFiles, filePathMatches } from './utils'; export const activeFile = (state) => state.openFiles.find((file) => file.active) || null; @@ -153,14 +160,47 @@ export const getDiffInfo = (state, getters) => (path) => { export const findProjectPermissions = (state, getters) => (projectId) => getters.findProject(projectId)?.userPermissions || DEFAULT_PERMISSIONS; +export const findPushRules = (state, getters) => (projectId) => + getters.findProject(projectId)?.pushRules || {}; + export const canReadMergeRequests = (state, getters) => Boolean(getters.findProjectPermissions(state.currentProjectId)[PERMISSION_READ_MR]); export const canCreateMergeRequests = (state, getters) => Boolean(getters.findProjectPermissions(state.currentProjectId)[PERMISSION_CREATE_MR]); -export const canPushCode = (state, getters) => - Boolean(getters.findProjectPermissions(state.currentProjectId)[PERMISSION_PUSH_CODE]); +/** + * Returns an object with `isAllowed` and `message` based on why the user cant push code + */ +export const canPushCodeStatus = (state, getters) => { + const canPushCode = getters.findProjectPermissions(state.currentProjectId)[PERMISSION_PUSH_CODE]; + const rejectUnsignedCommits = getters.findPushRules(state.currentProjectId)[ + PUSH_RULE_REJECT_UNSIGNED_COMMITS + ]; + + if (rejectUnsignedCommits) { + return { + isAllowed: false, + message: MSG_CANNOT_PUSH_UNSIGNED, + messageShort: MSG_CANNOT_PUSH_UNSIGNED_SHORT, + }; + } + if (!canPushCode) { + return { + isAllowed: false, + message: MSG_CANNOT_PUSH_CODE, + messageShort: MSG_CANNOT_PUSH_CODE_SHORT, + }; + } + + return { + isAllowed: true, + message: '', + messageShort: '', + }; +}; + +export const canPushCode = (state, getters) => getters.canPushCodeStatus.isAllowed; export const entryExists = (state) => (path) => Boolean(state.entries[path] && !state.entries[path].deleted); diff --git a/changelogs/unreleased/213581-ide-alert-user-when-reject-unsigned-commits.yml b/changelogs/unreleased/213581-ide-alert-user-when-reject-unsigned-commits.yml new file mode 100644 index 00000000000000..62ba5d7828d0f6 --- /dev/null +++ b/changelogs/unreleased/213581-ide-alert-user-when-reject-unsigned-commits.yml @@ -0,0 +1,5 @@ +--- +title: Web IDE disallow commit when project has 'reject unsigned commits' rule +merge_request: 54166 +author: +type: changed diff --git a/ee/app/assets/javascripts/ide/queries/get_ide_project.query.graphql b/ee/app/assets/javascripts/ide/queries/get_ide_project.query.graphql new file mode 100644 index 00000000000000..41fefa9240ccc8 --- /dev/null +++ b/ee/app/assets/javascripts/ide/queries/get_ide_project.query.graphql @@ -0,0 +1,10 @@ +#import "~/ide/queries/ide_project.fragment.graphql" + +query getIdeProject($projectPath: ID!) { + project(fullPath: $projectPath) { + ...IdeProject + pushRules { + rejectUnsignedCommits + } + } +} diff --git a/ee/spec/features/ide/user_opens_ide_spec.rb b/ee/spec/features/ide/user_opens_ide_spec.rb new file mode 100644 index 00000000000000..1ecaa6eadb529e --- /dev/null +++ b/ee/spec/features/ide/user_opens_ide_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'EE user opens IDE', :js do + include WebIdeSpecHelpers + + let_it_be(:unsigned_commits_warning) { 'This project does not accept unsigned commits.' } + + let(:project) { create(:project, :repository) } + let(:user) { project.owner } + + before do + stub_licensed_features(push_rules: true) + stub_licensed_features(reject_unsigned_commits: true) + sign_in(user) + end + + context 'default' do + before do + ide_visit(project) + end + + it 'does not show warning' do + expect(page).not_to have_text(unsigned_commits_warning) + end + end + + context 'when has reject_unsigned_commit push rule' do + before do + create(:push_rule, project: project, reject_unsigned_commits: true) + + ide_visit(project) + end + + it 'shows warning' do + expect(page).to have_text(unsigned_commits_warning) + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 9d3e6adb99bf49..3b9ccefa390590 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -33622,6 +33622,12 @@ msgstr "" msgid "WebIDE|Merge request" msgstr "" +msgid "WebIDE|This project does not accept unsigned commits." +msgstr "" + +msgid "WebIDE|This project does not accept unsigned commits. You will not be able to commit your changes through the Web IDE." +msgstr "" + msgid "WebIDE|You need permission to edit files directly in this project." msgstr "" diff --git a/spec/frontend/ide/components/commit_sidebar/form_spec.js b/spec/frontend/ide/components/commit_sidebar/form_spec.js index 2b567816ce895c..083a2a73b24595 100644 --- a/spec/frontend/ide/components/commit_sidebar/form_spec.js +++ b/spec/frontend/ide/components/commit_sidebar/form_spec.js @@ -14,6 +14,7 @@ import { createBranchChangedCommitError, branchAlreadyExistsCommitError, } from '~/ide/lib/errors'; +import { MSG_CANNOT_PUSH_CODE_SHORT } from '~/ide/messages'; import { createStore } from '~/ide/stores'; import { COMMIT_TO_NEW_BRANCH } from '~/ide/stores/modules/commit/constants'; @@ -84,8 +85,8 @@ describe('IDE commit form', () => { ${'when there are no changes'} | ${[]} | ${{ pushCode: true }} | ${goToEditView} | ${findBeginCommitButtonData} | ${true} | ${''} ${'when there are changes'} | ${['test']} | ${{ pushCode: true }} | ${goToEditView} | ${findBeginCommitButtonData} | ${false} | ${''} ${'when there are changes'} | ${['test']} | ${{ pushCode: true }} | ${goToCommitView} | ${findCommitButtonData} | ${false} | ${''} - ${'when user cannot push'} | ${['test']} | ${{ pushCode: false }} | ${goToEditView} | ${findBeginCommitButtonData} | ${true} | ${CommitForm.MSG_CANNOT_PUSH_CODE} - ${'when user cannot push'} | ${['test']} | ${{ pushCode: false }} | ${goToCommitView} | ${findCommitButtonData} | ${true} | ${CommitForm.MSG_CANNOT_PUSH_CODE} + ${'when user cannot push'} | ${['test']} | ${{ pushCode: false }} | ${goToEditView} | ${findBeginCommitButtonData} | ${true} | ${MSG_CANNOT_PUSH_CODE_SHORT} + ${'when user cannot push'} | ${['test']} | ${{ pushCode: false }} | ${goToCommitView} | ${findCommitButtonData} | ${true} | ${MSG_CANNOT_PUSH_CODE_SHORT} `('$desc', ({ stagedFiles, userPermissions, viewFn, buttonFn, disabled, tooltip }) => { beforeEach(async () => { store.state.stagedFiles = stagedFiles; diff --git a/spec/frontend/ide/components/ide_spec.js b/spec/frontend/ide/components/ide_spec.js index c9d19c18d0398d..bd251f78654484 100644 --- a/spec/frontend/ide/components/ide_spec.js +++ b/spec/frontend/ide/components/ide_spec.js @@ -4,6 +4,7 @@ import Vuex from 'vuex'; import waitForPromises from 'helpers/wait_for_promises'; import ErrorMessage from '~/ide/components/error_message.vue'; import Ide from '~/ide/components/ide.vue'; +import { MSG_CANNOT_PUSH_CODE } from '~/ide/messages'; import { createStore } from '~/ide/stores'; import { file } from '../helpers'; import { projectData } from '../mock_data'; @@ -158,7 +159,7 @@ describe('WebIDE', () => { expect(findAlert().props()).toMatchObject({ dismissible: false, }); - expect(findAlert().text()).toBe(Ide.MSG_CANNOT_PUSH_CODE); + expect(findAlert().text()).toBe(MSG_CANNOT_PUSH_CODE); }); it.each` diff --git a/spec/frontend/ide/services/index_spec.js b/spec/frontend/ide/services/index_spec.js index 678d58cba34903..3503834e24b764 100644 --- a/spec/frontend/ide/services/index_spec.js +++ b/spec/frontend/ide/services/index_spec.js @@ -1,7 +1,7 @@ import axios from 'axios'; import MockAdapter from 'axios-mock-adapter'; +import getIdeProject from 'ee_else_ce/ide/queries/get_ide_project.query.graphql'; import Api from '~/api'; -import getUserPermissions from '~/ide/queries/getUserPermissions.query.graphql'; import services from '~/ide/services'; import { query } from '~/ide/services/gql'; import { escapeFileUrl } from '~/lib/utils/url_utility'; @@ -228,7 +228,7 @@ describe('IDE services', () => { expect(response).toEqual({ data: { ...projectData, ...gqlProjectData } }); expect(Api.project).toHaveBeenCalledWith(TEST_PROJECT_ID); expect(query).toHaveBeenCalledWith({ - query: getUserPermissions, + query: getIdeProject, variables: { projectPath: TEST_PROJECT_ID, }, diff --git a/spec/frontend/ide/stores/getters_spec.js b/spec/frontend/ide/stores/getters_spec.js index 450f559202665c..6b66c87e205a08 100644 --- a/spec/frontend/ide/stores/getters_spec.js +++ b/spec/frontend/ide/stores/getters_spec.js @@ -1,7 +1,17 @@ import { TEST_HOST } from 'helpers/test_constants'; +import { + DEFAULT_PERMISSIONS, + PERMISSION_PUSH_CODE, + PUSH_RULE_REJECT_UNSIGNED_COMMITS, +} from '~/ide/constants'; +import { + MSG_CANNOT_PUSH_CODE, + MSG_CANNOT_PUSH_CODE_SHORT, + MSG_CANNOT_PUSH_UNSIGNED, + MSG_CANNOT_PUSH_UNSIGNED_SHORT, +} from '~/ide/messages'; import { createStore } from '~/ide/stores'; import * as getters from '~/ide/stores/getters'; -import { DEFAULT_PERMISSIONS } from '../../../../app/assets/javascripts/ide/constants'; import { file } from '../helpers'; const TEST_PROJECT_ID = 'test_project'; @@ -385,22 +395,23 @@ describe('IDE store getters', () => { ); }); - describe('findProjectPermissions', () => { - it('returns false if project not found', () => { - expect(localStore.getters.findProjectPermissions(TEST_PROJECT_ID)).toEqual( - DEFAULT_PERMISSIONS, - ); + describe.each` + getterName | projectField | defaultValue + ${'findProjectPermissions'} | ${'userPermissions'} | ${DEFAULT_PERMISSIONS} + ${'findPushRules'} | ${'pushRules'} | ${{}} + `('$getterName', ({ getterName, projectField, defaultValue }) => { + const callGetter = (...args) => localStore.getters[getterName](...args); + + it('returns default if project not found', () => { + expect(callGetter(TEST_PROJECT_ID)).toEqual(defaultValue); }); - it('finds permission in given project', () => { - const userPermissions = { - readMergeRequest: true, - createMergeRequestsIn: false, - }; + it('finds field in given project', () => { + const obj = { test: 'foo' }; - localState.projects[TEST_PROJECT_ID] = { userPermissions }; + localState.projects[TEST_PROJECT_ID] = { [projectField]: obj }; - expect(localStore.getters.findProjectPermissions(TEST_PROJECT_ID)).toBe(userPermissions); + expect(callGetter(TEST_PROJECT_ID)).toBe(obj); }); }); @@ -408,7 +419,6 @@ describe('IDE store getters', () => { getterName | permissionKey ${'canReadMergeRequests'} | ${'readMergeRequest'} ${'canCreateMergeRequests'} | ${'createMergeRequestIn'} - ${'canPushCode'} | ${'pushCode'} `('$getterName', ({ getterName, permissionKey }) => { it.each([true, false])('finds permission for current project (%s)', (val) => { localState.projects[TEST_PROJECT_ID] = { @@ -422,6 +432,38 @@ describe('IDE store getters', () => { }); }); + describe('canPushCodeStatus', () => { + it.each` + pushCode | rejectUnsignedCommits | expected + ${true} | ${false} | ${{ isAllowed: true, message: '', messageShort: '' }} + ${false} | ${false} | ${{ isAllowed: false, message: MSG_CANNOT_PUSH_CODE, messageShort: MSG_CANNOT_PUSH_CODE_SHORT }} + ${false} | ${true} | ${{ isAllowed: false, message: MSG_CANNOT_PUSH_UNSIGNED, messageShort: MSG_CANNOT_PUSH_UNSIGNED_SHORT }} + `( + 'with pushCode="$pushCode" and rejectUnsignedCommits="$rejectUnsignedCommits"', + ({ pushCode, rejectUnsignedCommits, expected }) => { + localState.projects[TEST_PROJECT_ID] = { + pushRules: { + [PUSH_RULE_REJECT_UNSIGNED_COMMITS]: rejectUnsignedCommits, + }, + userPermissions: { + [PERMISSION_PUSH_CODE]: pushCode, + }, + }; + localState.currentProjectId = TEST_PROJECT_ID; + + expect(localStore.getters.canPushCodeStatus).toEqual(expected); + }, + ); + }); + + describe('canPushCode', () => { + it.each([true, false])('with canPushCodeStatus.isAllowed = $s', (isAllowed) => { + const canPushCodeStatus = { isAllowed }; + + expect(getters.canPushCode({}, { canPushCodeStatus })).toBe(isAllowed); + }); + }); + describe('entryExists', () => { beforeEach(() => { localState.entries = { -- GitLab