Commit 7014a737 authored by Kamil Trzciński's avatar Kamil Trzciński

Merge branch '29943-environment-folder' into 'security-9-5'

Do not use `location.pathname` when accessing environments folders

See merge request !2147
parents 54ede0b8 0a1a50d2
...@@ -111,11 +111,11 @@ export default { ...@@ -111,11 +111,11 @@ export default {
}, },
methods: { methods: {
toggleFolder(folder, folderUrl) { toggleFolder(folder) {
this.store.toggleFolder(folder); this.store.toggleFolder(folder);
if (!folder.isOpen) { if (!folder.isOpen) {
this.fetchChildEnvironments(folder, folderUrl, true); this.fetchChildEnvironments(folder, true);
} }
}, },
...@@ -143,10 +143,10 @@ export default { ...@@ -143,10 +143,10 @@ export default {
.catch(this.errorCallback); .catch(this.errorCallback);
}, },
fetchChildEnvironments(folder, folderUrl, showLoader = false) { fetchChildEnvironments(folder, showLoader = false) {
this.store.updateEnvironmentProp(folder, 'isLoadingFolderContent', showLoader); this.store.updateEnvironmentProp(folder, 'isLoadingFolderContent', showLoader);
this.service.getFolderContent(folderUrl) this.service.getFolderContent(folder.folder_path)
.then(resp => resp.json()) .then(resp => resp.json())
.then(response => this.store.setfolderContent(folder, response.environments)) .then(response => this.store.setfolderContent(folder, response.environments))
.then(() => this.store.updateEnvironmentProp(folder, 'isLoadingFolderContent', false)) .then(() => this.store.updateEnvironmentProp(folder, 'isLoadingFolderContent', false))
...@@ -173,12 +173,7 @@ export default { ...@@ -173,12 +173,7 @@ export default {
// We need to verify if any folder is open to also update it // We need to verify if any folder is open to also update it
const openFolders = this.store.getOpenFolders(); const openFolders = this.store.getOpenFolders();
if (openFolders.length) { if (openFolders.length) {
openFolders.forEach((folder) => { openFolders.forEach(folder => this.fetchChildEnvironments(folder));
// TODO - Move this to the backend
const folderUrl = `${window.location.pathname}/folders/${folder.folderName}`;
return this.fetchChildEnvironments(folder, folderUrl);
});
} }
}, },
......
...@@ -410,20 +410,11 @@ export default { ...@@ -410,20 +410,11 @@ export default {
this.hasStopAction || this.hasStopAction ||
this.canRetry; this.canRetry;
}, },
/**
* Constructs folder URL based on the current location and the folder id.
*
* @return {String}
*/
folderUrl() {
return `${window.location.pathname}/folders/${this.model.folderName}`;
},
}, },
methods: { methods: {
onClickFolder() { onClickFolder() {
eventHub.$emit('toggleFolder', this.model, this.folderUrl); eventHub.$emit('toggleFolder', this.model);
}, },
}, },
}; };
......
...@@ -82,12 +82,7 @@ class Environment < ActiveRecord::Base ...@@ -82,12 +82,7 @@ class Environment < ActiveRecord::Base
def set_environment_type def set_environment_type
names = name.split('/') names = name.split('/')
self.environment_type = self.environment_type = names.many? ? names.first : nil
if names.many?
names.first
else
nil
end
end end
def includes_commit?(commit) def includes_commit?(commit)
...@@ -101,7 +96,7 @@ class Environment < ActiveRecord::Base ...@@ -101,7 +96,7 @@ class Environment < ActiveRecord::Base
end end
def update_merge_request_metrics? def update_merge_request_metrics?
(environment_type || name) == "production" folder_name == "production"
end end
def first_deployment_for(commit) def first_deployment_for(commit)
...@@ -223,6 +218,10 @@ class Environment < ActiveRecord::Base ...@@ -223,6 +218,10 @@ class Environment < ActiveRecord::Base
format: :json) format: :json)
end end
def folder_name
self.environment_type || self.name
end
private private
# Slugifying a name may remove the uniqueness guarantee afforded by it being # Slugifying a name may remove the uniqueness guarantee afforded by it being
......
...@@ -26,5 +26,9 @@ class EnvironmentEntity < Grape::Entity ...@@ -26,5 +26,9 @@ class EnvironmentEntity < Grape::Entity
terminal_project_environment_path(environment.project, environment) terminal_project_environment_path(environment.project, environment)
end end
expose :folder_path do |environment|
folder_project_environments_path(environment.project, environment.folder_name)
end
expose :created_at, :updated_at expose :created_at, :updated_at
end end
...@@ -36,9 +36,9 @@ class EnvironmentSerializer < BaseSerializer ...@@ -36,9 +36,9 @@ class EnvironmentSerializer < BaseSerializer
private private
def itemize(resource) def itemize(resource)
items = resource.order('folder_name ASC') items = resource.order('folder ASC')
.group('COALESCE(environment_type, name)') .group('COALESCE(environment_type, name)')
.select('COALESCE(environment_type, name) AS folder_name', .select('COALESCE(environment_type, name) AS folder',
'COUNT(*) AS size', 'MAX(id) AS last_id') 'COUNT(*) AS size', 'MAX(id) AS last_id')
# It makes a difference when you call `paginate` method, because # It makes a difference when you call `paginate` method, because
...@@ -49,7 +49,7 @@ class EnvironmentSerializer < BaseSerializer ...@@ -49,7 +49,7 @@ class EnvironmentSerializer < BaseSerializer
environments = resource.where(id: items.map(&:last_id)).index_by(&:id) environments = resource.where(id: items.map(&:last_id)).index_by(&:id)
items.map do |item| items.map do |item|
Item.new(item.folder_name, item.size, environments[item.last_id]) Item.new(item.folder, item.size, environments[item.last_id])
end end
end end
end end
---
title: Resolve CSRF token leakage via pathname manipulation on environments page
merge_request:
author:
...@@ -10,26 +10,23 @@ feature 'Environments page', :js do ...@@ -10,26 +10,23 @@ feature 'Environments page', :js do
sign_in(user) sign_in(user)
end end
given!(:environment) { }
given!(:deployment) { }
given!(:action) { }
before do
visit_environments(project)
end
describe 'page tabs' do describe 'page tabs' do
scenario 'shows "Available" and "Stopped" tab with links' do it 'shows "Available" and "Stopped" tab with links' do
visit_environments(project)
expect(page).to have_link('Available') expect(page).to have_link('Available')
expect(page).to have_link('Stopped') expect(page).to have_link('Stopped')
end end
describe 'with one available environment' do describe 'with one available environment' do
given(:environment) { create(:environment, project: project, state: :available) } before do
create(:environment, project: project, state: :available)
end
describe 'in available tab page' do describe 'in available tab page' do
it 'should show one environment' do it 'should show one environment' do
visit project_environments_path(project, scope: 'available') visit_environments(project, scope: 'available')
expect(page).to have_css('.environments-container') expect(page).to have_css('.environments-container')
expect(page.all('.environment-name').length).to eq(1) expect(page.all('.environment-name').length).to eq(1)
end end
...@@ -37,7 +34,8 @@ feature 'Environments page', :js do ...@@ -37,7 +34,8 @@ feature 'Environments page', :js do
describe 'in stopped tab page' do describe 'in stopped tab page' do
it 'should show no environments' do it 'should show no environments' do
visit project_environments_path(project, scope: 'stopped') visit_environments(project, scope: 'stopped')
expect(page).to have_css('.environments-container') expect(page).to have_css('.environments-container')
expect(page).to have_content('You don\'t have any environments right now') expect(page).to have_content('You don\'t have any environments right now')
end end
...@@ -45,11 +43,14 @@ feature 'Environments page', :js do ...@@ -45,11 +43,14 @@ feature 'Environments page', :js do
end end
describe 'with one stopped environment' do describe 'with one stopped environment' do
given(:environment) { create(:environment, project: project, state: :stopped) } before do
create(:environment, project: project, state: :stopped)
end
describe 'in available tab page' do describe 'in available tab page' do
it 'should show no environments' do it 'should show no environments' do
visit project_environments_path(project, scope: 'available') visit_environments(project, scope: 'available')
expect(page).to have_css('.environments-container') expect(page).to have_css('.environments-container')
expect(page).to have_content('You don\'t have any environments right now') expect(page).to have_content('You don\'t have any environments right now')
end end
...@@ -57,7 +58,8 @@ feature 'Environments page', :js do ...@@ -57,7 +58,8 @@ feature 'Environments page', :js do
describe 'in stopped tab page' do describe 'in stopped tab page' do
it 'should show one environment' do it 'should show one environment' do
visit project_environments_path(project, scope: 'stopped') visit_environments(project, scope: 'stopped')
expect(page).to have_css('.environments-container') expect(page).to have_css('.environments-container')
expect(page.all('.environment-name').length).to eq(1) expect(page.all('.environment-name').length).to eq(1)
end end
...@@ -66,86 +68,84 @@ feature 'Environments page', :js do ...@@ -66,86 +68,84 @@ feature 'Environments page', :js do
end end
context 'without environments' do context 'without environments' do
scenario 'does show no environments' do before do
expect(page).to have_content('You don\'t have any environments right now.') visit_environments(project)
end end
scenario 'does show 0 as counter for environments in both tabs' do it 'does not show environments and counters are set to zero' do
expect(page).to have_content('You don\'t have any environments right now.')
expect(page.find('.js-available-environments-count').text).to eq('0') expect(page.find('.js-available-environments-count').text).to eq('0')
expect(page.find('.js-stopped-environments-count').text).to eq('0') expect(page.find('.js-stopped-environments-count').text).to eq('0')
end end
end end
describe 'when showing the environment' do describe 'environments table' do
given(:environment) { create(:environment, project: project) } given!(:environment) do
create(:environment, project: project, state: :available)
scenario 'does show environment name' do
expect(page).to have_link(environment.name)
end
scenario 'does show number of available and stopped environments' do
expect(page.find('.js-available-environments-count').text).to eq('1')
expect(page.find('.js-stopped-environments-count').text).to eq('0')
end end
context 'without deployments' do context 'when there are no deployments' do
scenario 'does show no deployments' do before do
expect(page).to have_content('No deployments yet') visit_environments(project)
end end
context 'for available environment' do it 'shows environments names and counters' do
given(:environment) { create(:environment, project: project, state: :available) } expect(page).to have_link(environment.name)
scenario 'does not shows stop button' do expect(page.find('.js-available-environments-count').text).to eq('1')
expect(page).not_to have_selector('.stop-env-link') expect(page.find('.js-stopped-environments-count').text).to eq('0')
end
end end
context 'for stopped environment' do it 'does not show deployments' do
given(:environment) { create(:environment, project: project, state: :stopped) } expect(page).to have_content('No deployments yet')
end
scenario 'does not shows stop button' do it 'does not show stip button when environment is not stoppable' do
expect(page).not_to have_selector('.stop-env-link') expect(page).not_to have_selector('.stop-env-link')
end
end end
end end
context 'with deployments' do context 'when there are deployments' do
given(:project) { create(:project, :repository) } given(:project) { create(:project, :repository) }
given(:deployment) do given!(:deployment) do
create(:deployment, environment: environment, create(:deployment, environment: environment,
sha: project.commit.id) sha: project.commit.id)
end end
scenario 'does show deployment SHA' do it 'shows deployment SHA and internal ID' do
expect(page).to have_link(deployment.short_sha) visit_environments(project)
end
scenario 'does show deployment internal id' do expect(page).to have_link(deployment.short_sha)
expect(page).to have_content(deployment.iid) expect(page).to have_content(deployment.iid)
end end
context 'with build and manual actions' do context 'when builds and manual actions are present' do
given(:pipeline) { create(:ci_pipeline, project: project) } given!(:pipeline) { create(:ci_pipeline, project: project) }
given(:build) { create(:ci_build, pipeline: pipeline) } given!(:build) { create(:ci_build, pipeline: pipeline) }
given(:action) do given!(:action) do
create(:ci_build, :manual, pipeline: pipeline, name: 'deploy to production') create(:ci_build, :manual, pipeline: pipeline, name: 'deploy to production')
end end
given(:deployment) do given!(:deployment) do
create(:deployment, environment: environment, create(:deployment, environment: environment,
deployable: build, deployable: build,
sha: project.commit.id) sha: project.commit.id)
end end
scenario 'does show a play button' do before do
visit_environments(project)
end
it 'shows a play button' do
find('.js-dropdown-play-icon-container').click find('.js-dropdown-play-icon-container').click
expect(page).to have_content(action.name.humanize) expect(page).to have_content(action.name.humanize)
end end
scenario 'does allow to play manual action', js: true do it 'allows to play a manual action', js: true do
expect(action).to be_manual expect(action).to be_manual
find('.js-dropdown-play-icon-container').click find('.js-dropdown-play-icon-container').click
...@@ -155,19 +155,19 @@ feature 'Environments page', :js do ...@@ -155,19 +155,19 @@ feature 'Environments page', :js do
.not_to change { Ci::Pipeline.count } .not_to change { Ci::Pipeline.count }
end end
scenario 'does show build name and id' do it 'shows build name and id' do
expect(page).to have_link("#{build.name} ##{build.id}") expect(page).to have_link("#{build.name} ##{build.id}")
end end
scenario 'does not show stop button' do it 'shows a stop button' do
expect(page).not_to have_selector('.stop-env-link') expect(page).not_to have_selector('.stop-env-link')
end end
scenario 'does not show external link button' do it 'does not show external link button' do
expect(page).not_to have_css('external-url') expect(page).not_to have_css('external-url')
end end
scenario 'does not show terminal button' do it 'does not show terminal button' do
expect(page).not_to have_terminal_button expect(page).not_to have_terminal_button
end end
...@@ -176,7 +176,7 @@ feature 'Environments page', :js do ...@@ -176,7 +176,7 @@ feature 'Environments page', :js do
given(:build) { create(:ci_build, pipeline: pipeline) } given(:build) { create(:ci_build, pipeline: pipeline) }
given(:deployment) { create(:deployment, environment: environment, deployable: build) } given(:deployment) { create(:deployment, environment: environment, deployable: build) }
scenario 'does show an external link button' do it 'shows an external link button' do
expect(page).to have_link(nil, href: environment.external_url) expect(page).to have_link(nil, href: environment.external_url)
end end
end end
...@@ -192,34 +192,34 @@ feature 'Environments page', :js do ...@@ -192,34 +192,34 @@ feature 'Environments page', :js do
on_stop: 'close_app') on_stop: 'close_app')
end end
scenario 'does show stop button' do it 'shows a stop button' do
expect(page).to have_selector('.stop-env-link') expect(page).to have_selector('.stop-env-link')
end end
context 'for reporter' do context 'when user is a reporter' do
let(:role) { :reporter } let(:role) { :reporter }
scenario 'does not show stop button' do it 'does not show stop button' do
expect(page).not_to have_selector('.stop-env-link') expect(page).not_to have_selector('.stop-env-link')
end end
end end
end end
context 'with terminal' do context 'when kubernetes terminal is available' do
let(:project) { create(:kubernetes_project, :test_repo) } let(:project) { create(:kubernetes_project, :test_repo) }
context 'for project master' do context 'for project master' do
let(:role) { :master } let(:role) { :master }
scenario 'it shows the terminal button' do it 'shows the terminal button' do
expect(page).to have_terminal_button expect(page).to have_terminal_button
end end
end end
context 'for developer' do context 'when user is a developer' do
let(:role) { :developer } let(:role) { :developer }
scenario 'does not show terminal button' do it 'does not show terminal button' do
expect(page).not_to have_terminal_button expect(page).not_to have_terminal_button
end end
end end
...@@ -228,59 +228,77 @@ feature 'Environments page', :js do ...@@ -228,59 +228,77 @@ feature 'Environments page', :js do
end end
end end
scenario 'does have a New environment button' do it 'does have a new environment button' do
visit_environments(project)
expect(page).to have_link('New environment') expect(page).to have_link('New environment')
end end
describe 'when creating a new environment' do describe 'creating a new environment' do
before do before do
visit_environments(project) visit_environments(project)
end end
context 'when logged as developer' do context 'user is a developer' do
before do given(:role) { :developer }
within(".top-area") do
click_link 'New environment'
end
end
context 'for valid name' do scenario 'developer creates a new environment with a valid name' do
before do within(".top-area") { click_link 'New environment' }
fill_in('Name', with: 'production') fill_in('Name', with: 'production')
click_on 'Save' click_on 'Save'
end
scenario 'does create a new pipeline' do expect(page).to have_content('production')
expect(page).to have_content('production')
end
end end
context 'for invalid name' do scenario 'developer creates a new environmetn with invalid name' do
before do within(".top-area") { click_link 'New environment' }
fill_in('Name', with: 'name,with,commas') fill_in('Name', with: 'name,with,commas')
click_on 'Save' click_on 'Save'
end
scenario 'does show errors' do expect(page).to have_content('Name can contain only letters')
expect(page).to have_content('Name can contain only letters')
end
end end
end end
context 'when logged as reporter' do context 'user is a reporter' do
given(:role) { :reporter } given(:role) { :reporter }
scenario 'does not have a New environment link' do scenario 'reporters tries to create a new environment' do
expect(page).not_to have_link('New environment') expect(page).not_to have_link('New environment')
end end
end end
end end
describe 'environments folders' do
before do
create(:environment, project: project,
name: 'staging/review-1',
state: :available)
create(:environment, project: project,
name: 'staging/review-2',
state: :available)
end
scenario 'users unfurls an environment folder' do
visit_environments(project)
expect(page).not_to have_content 'review-1'
expect(page).not_to have_content 'review-2'
expect(page).to have_content 'staging 2'
within('.folder-row') do
find('.folder-name', text: 'staging').click
end
expect(page).to have_content 'review-1'
expect(page).to have_content 'review-2'
end
end
def have_terminal_button def have_terminal_button
have_link(nil, href: terminal_project_environment_path(project, environment)) have_link(nil, href: terminal_project_environment_path(project, environment))
end end
def visit_environments(project) def visit_environments(project, **opts)
visit project_environments_path(project) visit project_environments_path(project, **opts)
end end
end end
...@@ -54,6 +54,28 @@ describe Environment do ...@@ -54,6 +54,28 @@ describe Environment do
end end
end end
describe '#folder_name' do
context 'when it is inside a folder' do
subject(:environment) do
create(:environment, name: 'staging/review-1')
end
it 'returns a top-level folder name' do
expect(environment.folder_name).to eq 'staging'
end
end
context 'when the environment if a top-level item itself' do
subject(:environment) do
create(:environment, name: 'production')
end
it 'returns an environment name' do
expect(environment.folder_name).to eq 'production'
end
end
end
describe '#nullify_external_url' do describe '#nullify_external_url' do
it 'replaces a blank url with nil' do it 'replaces a blank url with nil' do
env = build(:environment, external_url: "") env = build(:environment, external_url: "")
......
...@@ -16,6 +16,10 @@ describe EnvironmentEntity do ...@@ -16,6 +16,10 @@ describe EnvironmentEntity do
expect(subject).to include(:id, :name, :state, :environment_path) expect(subject).to include(:id, :name, :state, :environment_path)
end end
it 'exposes folder path' do
expect(subject).to include(:folder_path)
end
context 'metrics disabled' do context 'metrics disabled' do
before do before do
allow(environment).to receive(:has_metrics?).and_return(false) allow(environment).to receive(:has_metrics?).and_return(false)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment