From 9e3d1aeb2aeb1e10ece2fbbf433deac486e51bf4 Mon Sep 17 00:00:00 2001 From: Aakriti Gupta Date: Fri, 2 Feb 2024 14:36:07 +0530 Subject: [PATCH 1/5] Remove GeoProxyFetchDirectToPrimary and GeoProxyDirectToPrimary flags --- internal/command/receivepack/receivepack.go | 14 -------------- internal/command/uploadpack/uploadpack.go | 10 ---------- internal/gitlabnet/accessverifier/client.go | 2 -- internal/gitlabnet/accessverifier/client_test.go | 2 -- 4 files changed, 28 deletions(-) diff --git a/internal/command/receivepack/receivepack.go b/internal/command/receivepack/receivepack.go index 2c75eb4d1..cbe91aa02 100644 --- a/internal/command/receivepack/receivepack.go +++ b/internal/command/receivepack/receivepack.go @@ -38,20 +38,6 @@ func (c *Command) Execute(ctx context.Context) (context.Context, error) { )) if response.IsCustomAction() { - // When `geo_proxy_direct_to_primary` feature flag is enabled, a Git over HTTP direct request - // to primary repo is performed instead of proxying the request through Gitlab Rails. - // After the feature flag is enabled by default and removed, - // custom action functionality will be removed along with it. - if response.Payload.Data.GeoProxyDirectToPrimary { - cmd := githttp.PushCommand{ - Config: c.Config, - ReadWriter: c.ReadWriter, - Response: response, - } - - return ctxWithLogData, cmd.Execute(ctx) - } - customAction := customaction.Command{ Config: c.Config, ReadWriter: c.ReadWriter, diff --git a/internal/command/uploadpack/uploadpack.go b/internal/command/uploadpack/uploadpack.go index f4d89bbee..229415775 100644 --- a/internal/command/uploadpack/uploadpack.go +++ b/internal/command/uploadpack/uploadpack.go @@ -39,16 +39,6 @@ func (c *Command) Execute(ctx context.Context) (context.Context, error) { ctxWithLogData := context.WithValue(ctx, "logData", logData) if response.IsCustomAction() { - if response.Payload.Data.GeoProxyFetchDirectToPrimary { - cmd := githttp.PullCommand{ - Config: c.Config, - ReadWriter: c.ReadWriter, - Response: response, - } - - return ctxWithLogData, cmd.Execute(ctx) - } - customAction := customaction.Command{ Config: c.Config, ReadWriter: c.ReadWriter, diff --git a/internal/gitlabnet/accessverifier/client.go b/internal/gitlabnet/accessverifier/client.go index 2cd14b882..4818beeac 100644 --- a/internal/gitlabnet/accessverifier/client.go +++ b/internal/gitlabnet/accessverifier/client.go @@ -48,8 +48,6 @@ type CustomPayloadData struct { PrimaryRepo string `json:"primary_repo"` UserId string `json:"gl_id,omitempty"` RequestHeaders map[string]string `json:"request_headers"` - GeoProxyDirectToPrimary bool `json:"geo_proxy_direct_to_primary"` - GeoProxyFetchDirectToPrimary bool `json:"geo_proxy_fetch_direct_to_primary"` GeoProxyFetchDirectToPrimaryWithOptions bool `json:"geo_proxy_fetch_direct_to_primary_with_options"` } diff --git a/internal/gitlabnet/accessverifier/client_test.go b/internal/gitlabnet/accessverifier/client_test.go index 4e9cc4d55..7ba197bdf 100644 --- a/internal/gitlabnet/accessverifier/client_test.go +++ b/internal/gitlabnet/accessverifier/client_test.go @@ -112,7 +112,6 @@ func TestGeoPushGetCustomAction(t *testing.T) { Action: "geo_proxy_to_primary", Data: CustomPayloadData{ ApiEndpoints: []string{"geo/proxy_git_ssh/info_refs_receive_pack", "geo/proxy_git_ssh/receive_pack"}, - GeoProxyDirectToPrimary: true, RequestHeaders: map[string]string{"Authorization": "Bearer token"}, Username: "custom", PrimaryRepo: "https://repo/path", @@ -143,7 +142,6 @@ func TestGeoPullGetCustomAction(t *testing.T) { Data: CustomPayloadData{ ApiEndpoints: []string{"geo/proxy_git_ssh/info_refs_upload_pack", "geo/proxy_git_ssh/upload_pack"}, Username: "custom", - GeoProxyFetchDirectToPrimary: true, PrimaryRepo: "https://repo/path", RequestHeaders: map[string]string{"Authorization": "Bearer token"}, }, -- GitLab From 3f188a7cafd49f8e682f0211828573b7c2ca461f Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Mon, 5 Feb 2024 08:10:49 +0000 Subject: [PATCH 2/5] Apply 2 suggestion(s) to 2 file(s) --- internal/command/receivepack/receivepack.go | 7 ++++--- internal/command/uploadpack/uploadpack.go | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/internal/command/receivepack/receivepack.go b/internal/command/receivepack/receivepack.go index cbe91aa02..4755838a8 100644 --- a/internal/command/receivepack/receivepack.go +++ b/internal/command/receivepack/receivepack.go @@ -38,12 +38,13 @@ func (c *Command) Execute(ctx context.Context) (context.Context, error) { )) if response.IsCustomAction() { - customAction := customaction.Command{ + cmd := githttp.PushCommand{ Config: c.Config, ReadWriter: c.ReadWriter, - EOFSent: true, + Response: response, } - return ctxWithLogData, customAction.Execute(ctx, response) + + return ctxWithLogData, cmd.Execute(ctx) } err = c.performGitalyCall(ctx, response) diff --git a/internal/command/uploadpack/uploadpack.go b/internal/command/uploadpack/uploadpack.go index 229415775..50343a855 100644 --- a/internal/command/uploadpack/uploadpack.go +++ b/internal/command/uploadpack/uploadpack.go @@ -39,12 +39,13 @@ func (c *Command) Execute(ctx context.Context) (context.Context, error) { ctxWithLogData := context.WithValue(ctx, "logData", logData) if response.IsCustomAction() { - customAction := customaction.Command{ + cmd := githttp.PullCommand{ Config: c.Config, ReadWriter: c.ReadWriter, - EOFSent: false, + Response: response, } - return ctxWithLogData, customAction.Execute(ctx, response) + + return ctxWithLogData, cmd.Execute(ctx) } stats, err := c.performGitalyCall(ctx, response) -- GitLab From 6e398ea3bc61aa1d5d1e895945dc42a5f3bc5518 Mon Sep 17 00:00:00 2001 From: Aakriti Gupta Date: Mon, 5 Feb 2024 11:12:21 +0000 Subject: [PATCH 3/5] Apply 2 suggestion(s) to 2 file(s) --- internal/command/receivepack/receivepack.go | 1 - internal/command/uploadpack/uploadpack.go | 1 - 2 files changed, 2 deletions(-) diff --git a/internal/command/receivepack/receivepack.go b/internal/command/receivepack/receivepack.go index 4755838a8..e39dfbdd3 100644 --- a/internal/command/receivepack/receivepack.go +++ b/internal/command/receivepack/receivepack.go @@ -9,7 +9,6 @@ import ( "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/githttp" "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/readwriter" "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/shared/accessverifier" - "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/shared/customaction" "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/shared/disallowedcommand" "gitlab.com/gitlab-org/gitlab-shell/v14/internal/config" ) diff --git a/internal/command/uploadpack/uploadpack.go b/internal/command/uploadpack/uploadpack.go index 50343a855..2167fa662 100644 --- a/internal/command/uploadpack/uploadpack.go +++ b/internal/command/uploadpack/uploadpack.go @@ -9,7 +9,6 @@ import ( "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/gitauditevent" "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/readwriter" "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/shared/accessverifier" - "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/shared/customaction" "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/shared/disallowedcommand" "gitlab.com/gitlab-org/gitlab-shell/v14/internal/config" ) -- GitLab From a4f8357a3c0480abdae8957e679b62c9e6da6a9d Mon Sep 17 00:00:00 2001 From: Aakriti Gupta Date: Mon, 5 Feb 2024 11:23:03 +0000 Subject: [PATCH 4/5] Remove custom action spec files --- ...tlab_shell_custom_git_receive_pack_spec.rb | 118 ------------------ ...itlab_shell_custom_git_upload_pack_spec.rb | 118 ------------------ 2 files changed, 236 deletions(-) delete mode 100644 spec/gitlab_shell_custom_git_receive_pack_spec.rb delete mode 100644 spec/gitlab_shell_custom_git_upload_pack_spec.rb diff --git a/spec/gitlab_shell_custom_git_receive_pack_spec.rb b/spec/gitlab_shell_custom_git_receive_pack_spec.rb deleted file mode 100644 index 1dce4bbab..000000000 --- a/spec/gitlab_shell_custom_git_receive_pack_spec.rb +++ /dev/null @@ -1,118 +0,0 @@ -require_relative 'spec_helper' - -require 'open3' -require 'json' -require 'base64' - -describe 'Custom bin/gitlab-shell git-receive-pack' do - include_context 'gitlab shell' - - let(:env) { {'SSH_CONNECTION' => 'fake', 'SSH_ORIGINAL_COMMAND' => 'git-receive-pack group/repo' } } - let(:divider) { "remote: ========================================================================\n" } - - before(:context) do - write_config("gitlab_url" => "http+unix://#{CGI.escape(tmp_socket_path)}") - end - - def mock_server(server) - server.mount_proc('/geo/proxy_git_ssh/info_refs_receive_pack') do |req, res| - res.content_type = 'application/json' - res.status = 200 - - res.body = {"result" => "#{Base64.encode64('custom')}"}.to_json - end - - server.mount_proc('/geo/proxy_git_ssh/receive_pack') do |req, res| - res.content_type = 'application/json' - res.status = 200 - - output = JSON.parse(req.body)['output'] - - res.body = {"result" => output}.to_json - end - - server.mount_proc('/api/v4/internal/allowed') do |req, res| - res.content_type = 'application/json' - - key_id = req.query['key_id'] || req.query['username'] - - unless key_id - body = JSON.parse(req.body) - key_id = body['key_id'] || body['username'].to_s - end - - case key_id - when '100', 'someone' then - res.status = 300 - body = { - "gl_id" => "user-100", - "status" => true, - "payload" => { - "action" => "geo_proxy_to_primary", - "data" => { - "api_endpoints" => ["/geo/proxy_git_ssh/info_refs_receive_pack", "/geo/proxy_git_ssh/receive_pack"], - "gl_username" => "custom", - "primary_repo" => "https://repo/path" - }, - }, - "gl_console_messages" => ["console", "message"] - } - res.body = body.to_json - else - res.status = 403 - end - end - end - - describe 'dialog for performing a custom action' do - context 'when API calls perform successfully' do - let(:remote_blank_line) { "remote: \n" } - def verify_successful_call!(cmd) - Open3.popen3(env, cmd) do |stdin, stdout, stderr| - expect(stderr.gets).to eq(remote_blank_line) - expect(stderr.gets).to eq("remote: console\n") - expect(stderr.gets).to eq("remote: message\n") - expect(stderr.gets).to eq(remote_blank_line) - - stdin.puts("0009input") - stdin.close - - expect(stdout.gets(6)).to eq("custom") - expect(stdout.flush.read).to eq("0009input") - end - end - - context 'when key is provided' do - let(:cmd) { "#{gitlab_shell_path} key-100" } - - it 'custom action is performed' do - verify_successful_call!(cmd) - end - end - - context 'when username is provided' do - let(:cmd) { "#{gitlab_shell_path} username-someone" } - - it 'custom action is performed' do - verify_successful_call!(cmd) - end - end - end - - context 'when API error occurs' do - let(:cmd) { "#{gitlab_shell_path} key-101" } - - it 'custom action is not performed' do - Open3.popen2e(env, cmd) do |stdin, stdout| - expect(stdout.gets).to eq("remote: \n") - expect(stdout.gets).to eq(divider) - expect(stdout.gets).to eq("remote: \n") - expect(stdout.gets).to eq("remote: Internal API error (403)\n") - expect(stdout.gets).to eq("remote: \n") - expect(stdout.gets).to eq(divider) - expect(stdout.gets).to eq("remote: \n") - end - end - end - end -end diff --git a/spec/gitlab_shell_custom_git_upload_pack_spec.rb b/spec/gitlab_shell_custom_git_upload_pack_spec.rb deleted file mode 100644 index 6770344a5..000000000 --- a/spec/gitlab_shell_custom_git_upload_pack_spec.rb +++ /dev/null @@ -1,118 +0,0 @@ -require_relative 'spec_helper' - -require 'open3' -require 'json' -require 'base64' - -describe 'Custom bin/gitlab-shell git-upload-pack' do - include_context 'gitlab shell' - - let(:env) { {'SSH_CONNECTION' => 'fake', 'SSH_ORIGINAL_COMMAND' => 'git-upload-pack group/repo' } } - let(:divider) { "remote: ========================================================================\n" } - - before(:context) do - write_config("gitlab_url" => "http+unix://#{CGI.escape(tmp_socket_path)}") - end - - def mock_server(server) - server.mount_proc('/geo/proxy_git_ssh/info_refs_upload_pack') do |req, res| - res.content_type = 'application/json' - res.status = 200 - - res.body = {"result" => "#{Base64.encode64('custom')}"}.to_json - end - - server.mount_proc('/geo/proxy_git_ssh/upload_pack') do |req, res| - res.content_type = 'application/json' - res.status = 200 - - output = JSON.parse(req.body)['output'] - - res.body = {"result" => output}.to_json - end - - server.mount_proc('/api/v4/internal/allowed') do |req, res| - res.content_type = 'application/json' - - key_id = req.query['key_id'] || req.query['username'] - - unless key_id - body = JSON.parse(req.body) - key_id = body['key_id'] || body['username'].to_s - end - - case key_id - when '100', 'someone' then - res.status = 300 - body = { - "gl_id" => "user-100", - "status" => true, - "payload" => { - "action" => "geo_proxy_to_primary", - "data" => { - "api_endpoints" => ["/geo/proxy_git_ssh/info_refs_upload_pack", "/geo/proxy_git_ssh/upload_pack"], - "gl_username" => "custom", - "primary_repo" => "https://repo/path" - }, - }, - "gl_console_messages" => ["console", "message"] - } - res.body = body.to_json - else - res.status = 403 - end - end - end - - describe 'dialog for performing a custom action' do - context 'when API calls perform successfully' do - let(:remote_blank_line) { "remote: \n" } - def verify_successful_call!(cmd) - Open3.popen3(env, cmd) do |stdin, stdout, stderr| - expect(stderr.gets).to eq(remote_blank_line) - expect(stderr.gets).to eq("remote: console\n") - expect(stderr.gets).to eq("remote: message\n") - expect(stderr.gets).to eq(remote_blank_line) - - stdin.puts("0032want 343d70886785dc1f98aaf70f3b4ca87c93a5d0dd\n") - stdin.close - - expect(stdout.gets(6)).to eq("custom") - expect(stdout.flush.read).to eq("0032want 343d70886785dc1f98aaf70f3b4ca87c93a5d0dd\n") - end - end - - context 'when key is provided' do - let(:cmd) { "#{gitlab_shell_path} key-100" } - - it 'custom action is performed' do - verify_successful_call!(cmd) - end - end - - context 'when username is provided' do - let(:cmd) { "#{gitlab_shell_path} username-someone" } - - it 'custom action is performed' do - verify_successful_call!(cmd) - end - end - end - - context 'when API error occurs' do - let(:cmd) { "#{gitlab_shell_path} key-101" } - - it 'custom action is not performed' do - Open3.popen2e(env, cmd) do |stdin, stdout| - expect(stdout.gets).to eq("remote: \n") - expect(stdout.gets).to eq(divider) - expect(stdout.gets).to eq("remote: \n") - expect(stdout.gets).to eq("remote: Internal API error (403)\n") - expect(stdout.gets).to eq("remote: \n") - expect(stdout.gets).to eq(divider) - expect(stdout.gets).to eq("remote: \n") - end - end - end - end -end -- GitLab From 20c672f58f89f912efa0f5b33c20415b29266093 Mon Sep 17 00:00:00 2001 From: Aakriti Gupta Date: Mon, 5 Feb 2024 11:34:38 +0000 Subject: [PATCH 5/5] Remove custom action test --- internal/command/receivepack/receivepack_test.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/internal/command/receivepack/receivepack_test.go b/internal/command/receivepack/receivepack_test.go index 78561d365..16cf830e7 100644 --- a/internal/command/receivepack/receivepack_test.go +++ b/internal/command/receivepack/receivepack_test.go @@ -38,14 +38,6 @@ func TestForbiddenAccess(t *testing.T) { require.Equal(t, "Disallowed by API call", err.Error()) } -func TestCustomReceivePack(t *testing.T) { - cmd, output := setup(t, "1", requesthandlers.BuildAllowedWithCustomActionsHandlers(t)) - - _, err := cmd.Execute(context.Background()) - require.NoError(t, err) - require.Equal(t, "customoutput", output.String()) -} - func setup(t *testing.T, keyId string, requests []testserver.TestRequestHandler) (*Command, *bytes.Buffer) { url := testserver.StartSocketHttpServer(t, requests) -- GitLab