Merge branch 'xanf-fix-404-on-import' into 'master'

Resolve "Importing project by user with developer rights results in 404"

See merge request gitlab-org/gitlab!35667

app/controllers/projects/imports_controller.rb

@@ -5,7 +5,8 @@ class Projects::ImportsController < Projects::ApplicationController
   include ImportUrlParams
 
   # Authorize
-  before_action :authorize_admin_project!
+  before_action :authorize_admin_project!, only: [:new, :create]
+  before_action :require_namespace_project_creation_permission, only: :show
   before_action :require_no_repo, only: [:new, :create]
   before_action :redirect_if_progress, only: [:new, :create]
   before_action :redirect_if_no_import, only: :show
@@ -51,6 +52,10 @@ class Projects::ImportsController < Projects::ApplicationController
     end
   end
 
+  def require_namespace_project_creation_permission
+    render_404 unless current_user.can?(:admin_project, @project) || current_user.can?(:create_projects, @project.namespace)
+  end
+
   def redirect_if_progress
     if @project.import_in_progress?
       redirect_to project_import_path(@project)

changelogs/unreleased/xanf-fix-404-on-import.yml

+---
+title: Fix 404 when importing project with developer permission
+merge_request: 35667
+author:
+type: fixed

spec/controllers/projects/imports_controller_spec.rb

@@ -8,33 +8,15 @@ RSpec.describe Projects::ImportsController do
 
   before do
     sign_in(user)
-    project.add_maintainer(user)
   end
 
   describe 'GET #show' do
-    context 'when repository does not exists' do
-      it 'renders template' do
-        get :show, params: { namespace_id: project.namespace.to_param, project_id: project }
-
-        expect(response).to render_template :show
-      end
-
-      it 'sets flash.now if params is present' do
-        get :show, params: { namespace_id: project.namespace.to_param, project_id: project, continue: { to: '/', notice_now: 'Started' } }
-
-        expect(flash.now[:notice]).to eq 'Started'
+    context 'when the user has maintainer rights' do
+      before do
+        project.add_maintainer(user)
       end
-    end
-
-    context 'when repository exists' do
-      let(:project) { create(:project_empty_repo, import_url: 'https://github.com/vim/vim.git') }
-      let(:import_state) { project.import_state }
-
-      context 'when import is in progress' do
-        before do
-          import_state.update(status: :started)
-        end
 
+      context 'when repository does not exists' do
         it 'renders template' do
           get :show, params: { namespace_id: project.namespace.to_param, project_id: project }
 
@@ -42,82 +24,138 @@ RSpec.describe Projects::ImportsController do
         end
 
         it 'sets flash.now if params is present' do
-          get :show, params: { namespace_id: project.namespace.to_param, project_id: project, continue: { to: '/', notice_now: 'In progress' } }
+          get :show, params: { namespace_id: project.namespace.to_param, project_id: project, continue: { to: '/', notice_now: 'Started' } }
 
-          expect(flash.now[:notice]).to eq 'In progress'
+          expect(flash.now[:notice]).to eq 'Started'
         end
       end
 
-      context 'when import failed' do
-        before do
-          import_state.update(status: :failed)
-        end
+      context 'when repository exists' do
+        let(:project) { create(:project_empty_repo, import_url: 'https://github.com/vim/vim.git') }
+        let(:import_state) { project.import_state }
 
-        it 'redirects to new_namespace_project_import_path' do
-          get :show, params: { namespace_id: project.namespace.to_param, project_id: project }
+        context 'when import is in progress' do
+          before do
+            import_state.update(status: :started)
+          end
 
-          expect(response).to redirect_to new_project_import_path(project)
-        end
-      end
+          it 'renders template' do
+            get :show, params: { namespace_id: project.namespace.to_param, project_id: project }
 
-      context 'when import finished' do
-        before do
-          import_state.update(status: :finished)
+            expect(response).to render_template :show
+          end
+
+          it 'sets flash.now if params is present' do
+            get :show, params: { namespace_id: project.namespace.to_param, project_id: project, continue: { to: '/', notice_now: 'In progress' } }
+
+            expect(flash.now[:notice]).to eq 'In progress'
+          end
         end
 
-        context 'when project is a fork' do
-          it 'redirects to namespace_project_path' do
-            allow_any_instance_of(Project).to receive(:forked?).and_return(true)
+        context 'when import failed' do
+          before do
+            import_state.update(status: :failed)
+          end
 
+          it 'redirects to new_namespace_project_import_path' do
             get :show, params: { namespace_id: project.namespace.to_param, project_id: project }
 
-            expect(flash[:notice]).to eq 'The project was successfully forked.'
-            expect(response).to redirect_to project_path(project)
+            expect(response).to redirect_to new_project_import_path(project)
           end
         end
 
-        context 'when project is external' do
-          it 'redirects to namespace_project_path' do
-            get :show, params: { namespace_id: project.namespace.to_param, project_id: project }
+        context 'when import finished' do
+          before do
+            import_state.update(status: :finished)
+          end
 
-            expect(flash[:notice]).to eq 'The project was successfully imported.'
-            expect(response).to redirect_to project_path(project)
+          context 'when project is a fork' do
+            it 'redirects to namespace_project_path' do
+              allow_any_instance_of(Project).to receive(:forked?).and_return(true)
+
+              get :show, params: { namespace_id: project.namespace.to_param, project_id: project }
+
+              expect(flash[:notice]).to eq 'The project was successfully forked.'
+              expect(response).to redirect_to project_path(project)
+            end
           end
-        end
 
-        context 'when continue params is present' do
-          let(:params) do
-            {
-              to: project_path(project),
-              notice: 'Finished'
-            }
+          context 'when project is external' do
+            it 'redirects to namespace_project_path' do
+              get :show, params: { namespace_id: project.namespace.to_param, project_id: project }
+
+              expect(flash[:notice]).to eq 'The project was successfully imported.'
+              expect(response).to redirect_to project_path(project)
+            end
           end
 
-          it 'redirects to internal params[:to]' do
-            get :show, params: { namespace_id: project.namespace.to_param, project_id: project, continue: params }
+          context 'when continue params is present' do
+            let(:params) do
+              {
+                to: project_path(project),
+                notice: 'Finished'
+              }
+            end
+
+            it 'redirects to internal params[:to]' do
+              get :show, params: { namespace_id: project.namespace.to_param, project_id: project, continue: params }
+
+              expect(flash[:notice]).to eq params[:notice]
+              expect(response).to redirect_to params[:to]
+            end
 
-            expect(flash[:notice]).to eq params[:notice]
-            expect(response).to redirect_to params[:to]
+            it 'does not redirect to external params[:to]' do
+              params[:to] = "//google.com"
+
+              get :show, params: { namespace_id: project.namespace.to_param, project_id: project, continue: params }
+              expect(response).not_to redirect_to params[:to]
+            end
           end
+        end
 
-          it 'does not redirect to external params[:to]' do
-            params[:to] = "//google.com"
+        context 'when import never happened' do
+          before do
+            import_state.update(status: :none)
+          end
 
-            get :show, params: { namespace_id: project.namespace.to_param, project_id: project, continue: params }
-            expect(response).not_to redirect_to params[:to]
+          it 'redirects to namespace_project_path' do
+            get :show, params: { namespace_id: project.namespace.to_param, project_id: project }
+
+            expect(response).to redirect_to project_path(project)
           end
         end
       end
+    end
+
+    context 'when project is in group' do
+      let(:project) { create(:project_empty_repo, import_url: 'https://github.com/vim/vim.git', namespace: group) }
+
+      context 'when user has developer access to group and import is in progress' do
+        let(:import_state) { project.import_state }
 
-      context 'when import never happened' do
         before do
-          import_state.update(status: :none)
+          group.add_developer(user)
+          import_state.update!(status: :started)
         end
 
-        it 'redirects to namespace_project_path' do
-          get :show, params: { namespace_id: project.namespace.to_param, project_id: project }
+        context 'when group allows developers to create projects' do
+          let(:group) { create(:group, project_creation_level: Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS) }
 
-          expect(response).to redirect_to project_path(project)
+          it 'renders template' do
+            get :show, params: { namespace_id: project.namespace.to_param, project_id: project }
+
+            expect(response).to render_template :show
+          end
+        end
+
+        context 'when group prohibits developers to create projects' do
+          let(:group) { create(:group, project_creation_level: Gitlab::Access::MAINTAINER_PROJECT_ACCESS) }
+
+          it 'returns 404 response' do
+            get :show, params: { namespace_id: project.namespace.to_param, project_id: project }
+
+            expect(response).to have_gitlab_http_status(:not_found)
+          end
         end
       end
     end
@@ -128,6 +166,7 @@ RSpec.describe Projects::ImportsController do
     let(:project) { create(:project) }
 
     before do
+      project.add_maintainer(user)
       allow(RepositoryImportWorker).to receive(:perform_async)
 
       post :create, params: { project: params, namespace_id: project.namespace.to_param, project_id: project }