Commit 6405877b authored by Stan Hu's avatar Stan Hu

Prevent accidental group deletion if path rename fails

If an update to the group path fails validation, the unsaved attributes
would still be presented because the controller would fall through and
call `render` on the unsaved group object. There was an attempt to reset
the path to the previous value, but this did not work because the Rails
form helper called `Group#to_param`, which loads the full route.

While it may have been intentional to retain the failed values, it's
better to be safe and reset all the values back to their original state.

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/241110
parent b86db507
...@@ -123,7 +123,7 @@ class GroupsController < Groups::ApplicationController ...@@ -123,7 +123,7 @@ class GroupsController < Groups::ApplicationController
if Groups::UpdateService.new(@group, current_user, group_params).execute if Groups::UpdateService.new(@group, current_user, group_params).execute
redirect_to edit_group_path(@group, anchor: params[:update_section]), notice: "Group '#{@group.name}' was successfully updated." redirect_to edit_group_path(@group, anchor: params[:update_section]), notice: "Group '#{@group.name}' was successfully updated."
else else
@group.path = @group.path_before_last_save || @group.path_was @group.reset
render action: "edit" render action: "edit"
end end
end end
......
---
title: Prevent accidental group deletion if path rename fails
merge_request: 40353
author:
type: fixed
...@@ -555,6 +555,21 @@ RSpec.describe GroupsController do ...@@ -555,6 +555,21 @@ RSpec.describe GroupsController do
end end
end end
context 'when there is a conflicting group path' do
render_views
let!(:conflict_group) { create(:group, path: SecureRandom.hex(12) ) }
let!(:old_name) { group.name }
it 'does not render references to the conflicting group' do
put :update, params: { id: group.to_param, group: { path: conflict_group.path } }
expect(response).to have_gitlab_http_status(:ok)
expect(group.reload.name).to eq(old_name)
expect(response.body).not_to include(conflict_group.path)
end
end
context 'when a project inside the group has container repositories' do context 'when a project inside the group has container repositories' do
before do before do
stub_container_registry_config(enabled: true) stub_container_registry_config(enabled: true)
......
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