Merge branch '173-remove-go-fallback-and-feature-flags' into 'master'

Remove feature flags and the fallback command

See merge request gitlab-org/gitlab-shell!336

go/cmd/gitlab-shell-authorized-keys-check/main.go

@@ -37,8 +37,6 @@ func main() {
 		os.Exit(1)
 	}
 
-	// The command will write to STDOUT on execution or replace the current
-	// process in case of the `fallback.Command`
 	if err = cmd.Execute(); err != nil {
 		fmt.Fprintf(readWriter.ErrOut, "%v\n", err)
 		os.Exit(1)

go/cmd/gitlab-shell-authorized-principals-check/main.go

@@ -37,8 +37,6 @@ func main() {
 		os.Exit(1)
 	}
 
-	// The command will write to STDOUT on execution or replace the current
-	// process in case of the `fallback.Command`
 	if err = cmd.Execute(); err != nil {
 		fmt.Fprintf(readWriter.ErrOut, "%v\n", err)
 		os.Exit(1)

go/cmd/gitlab-shell/main.go

@@ -37,8 +37,6 @@ func main() {
 		os.Exit(1)
 	}
 
-	// The command will write to STDOUT on execution or replace the current
-	// process in case of the `fallback.Command`
 	if err = cmd.Execute(); err != nil {
 		fmt.Fprintf(readWriter.ErrOut, "%v\n", err)
 		os.Exit(1)

go/internal/command/command.go

@@ -5,10 +5,10 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/authorizedprincipals"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/commandargs"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/discover"
-	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/fallback"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/lfsauthenticate"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/readwriter"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/receivepack"
+	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/shared/disallowedcommand"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/twofactorrecover"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/uploadarchive"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/uploadpack"
@@ -30,7 +30,7 @@ func New(e *executable.Executable, arguments []string, config *config.Config, re
 		return cmd, nil
 	}
 
-	return &fallback.Command{Executable: e, RootDir: config.RootDir, Args: args}, nil
+	return nil, disallowedcommand.Error
 }
 
 func buildCommand(e *executable.Executable, args commandargs.CommandArgs, config *config.Config, readWriter *readwriter.ReadWriter) Command {
@@ -47,10 +47,6 @@ func buildCommand(e *executable.Executable, args commandargs.CommandArgs, config
 }
 
 func buildShellCommand(args *commandargs.Shell, config *config.Config, readWriter *readwriter.ReadWriter) Command {
-	if !config.FeatureEnabled(string(args.CommandType)) {
-		return nil
-	}
-
 	switch args.CommandType {
 	case commandargs.Discover:
 		return &discover.Command{Config: config, Args: args, ReadWriter: readWriter}
@@ -70,17 +66,9 @@ func buildShellCommand(args *commandargs.Shell, config *config.Config, readWrite
 }
 
 func buildAuthorizedKeysCommand(args *commandargs.AuthorizedKeys, config *config.Config, readWriter *readwriter.ReadWriter) Command {
-	if !config.FeatureEnabled(executable.AuthorizedKeysCheck) {
-		return nil
-	}
-
 	return &authorizedkeys.Command{Config: config, Args: args, ReadWriter: readWriter}
 }
 
 func buildAuthorizedPrincipalsCommand(args *commandargs.AuthorizedPrincipals, config *config.Config, readWriter *readwriter.ReadWriter) Command {
-	if !config.FeatureEnabled(executable.AuthorizedPrincipalsCheck) {
-		return nil
-	}
-
 	return &authorizedprincipals.Command{Config: config, Args: args, ReadWriter: readWriter}
 }

go/internal/command/command_test.go

 package command
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -8,9 +9,9 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/authorizedkeys"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/authorizedprincipals"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/discover"
-	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/fallback"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/lfsauthenticate"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/receivepack"
+	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/shared/disallowedcommand"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/twofactorrecover"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/uploadarchive"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/uploadpack"
@@ -19,154 +20,77 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/testhelper"
 )
 
+var (
+	authorizedKeysExec       = &executable.Executable{Name: executable.AuthorizedKeysCheck}
+	authorizedPrincipalsExec = &executable.Executable{Name: executable.AuthorizedPrincipalsCheck}
+	gitlabShellExec          = &executable.Executable{Name: executable.GitlabShell}
+
+	basicConfig = &config.Config{GitlabUrl: "http+unix://gitlab.socket"}
+)
+
+func buildEnv(command string) map[string]string {
+	return map[string]string{
+		"SSH_CONNECTION":       "1",
+		"SSH_ORIGINAL_COMMAND": command,
+	}
+}
+
 func TestNew(t *testing.T) {
 	testCases := []struct {
 		desc         string
 		executable   *executable.Executable
-		config       *config.Config
 		environment  map[string]string
 		arguments    []string
 		expectedType interface{}
 	}{
 		{
-			desc:       "it returns a Discover command if the feature is enabled",
-			executable: &executable.Executable{Name: executable.GitlabShell},
-			config: &config.Config{
-				GitlabUrl: "http+unix://gitlab.socket",
-				Migration: config.MigrationConfig{Enabled: true, Features: []string{"discover"}},
-			},
-			environment: map[string]string{
-				"SSH_CONNECTION":       "1",
-				"SSH_ORIGINAL_COMMAND": "",
-			},
-			arguments:    []string{},
+			desc:         "it returns a Discover command",
+			executable:   gitlabShellExec,
+			environment:  buildEnv(""),
 			expectedType: &discover.Command{},
 		},
 		{
-			desc:       "it returns a Fallback command no feature is enabled",
-			executable: &executable.Executable{Name: executable.GitlabShell},
-			config: &config.Config{
-				GitlabUrl: "http+unix://gitlab.socket",
-				Migration: config.MigrationConfig{Enabled: false},
-			},
-			environment: map[string]string{
-				"SSH_CONNECTION":       "1",
-				"SSH_ORIGINAL_COMMAND": "",
-			},
-			arguments:    []string{},
-			expectedType: &fallback.Command{},
-		},
-		{
-			desc:       "it returns a TwoFactorRecover command if the feature is enabled",
-			executable: &executable.Executable{Name: executable.GitlabShell},
-			config: &config.Config{
-				GitlabUrl: "http+unix://gitlab.socket",
-				Migration: config.MigrationConfig{Enabled: true, Features: []string{"2fa_recovery_codes"}},
-			},
-			environment: map[string]string{
-				"SSH_CONNECTION":       "1",
-				"SSH_ORIGINAL_COMMAND": "2fa_recovery_codes",
-			},
-			arguments:    []string{},
+			desc:         "it returns a TwoFactorRecover command",
+			executable:   gitlabShellExec,
+			environment:  buildEnv("2fa_recovery_codes"),
 			expectedType: &twofactorrecover.Command{},
 		},
 		{
-			desc:       "it returns an LfsAuthenticate command if the feature is enabled",
-			executable: &executable.Executable{Name: executable.GitlabShell},
-			config: &config.Config{
-				GitlabUrl: "http+unix://gitlab.socket",
-				Migration: config.MigrationConfig{Enabled: true, Features: []string{"git-lfs-authenticate"}},
-			},
-			environment: map[string]string{
-				"SSH_CONNECTION":       "1",
-				"SSH_ORIGINAL_COMMAND": "git-lfs-authenticate",
-			},
-			arguments:    []string{},
+			desc:         "it returns an LfsAuthenticate command",
+			executable:   gitlabShellExec,
+			environment:  buildEnv("git-lfs-authenticate"),
 			expectedType: &lfsauthenticate.Command{},
 		},
 		{
-			desc:       "it returns a ReceivePack command if the feature is enabled",
-			executable: &executable.Executable{Name: executable.GitlabShell},
-			config: &config.Config{
-				GitlabUrl: "http+unix://gitlab.socket",
-				Migration: config.MigrationConfig{Enabled: true, Features: []string{"git-receive-pack"}},
-			},
-			environment: map[string]string{
-				"SSH_CONNECTION":       "1",
-				"SSH_ORIGINAL_COMMAND": "git-receive-pack",
-			},
-			arguments:    []string{},
+			desc:         "it returns a ReceivePack command",
+			executable:   gitlabShellExec,
+			environment:  buildEnv("git-receive-pack"),
 			expectedType: &receivepack.Command{},
 		},
 		{
-			desc:       "it returns an UploadPack command if the feature is enabled",
-			executable: &executable.Executable{Name: executable.GitlabShell},
-			config: &config.Config{
-				GitlabUrl: "http+unix://gitlab.socket",
-				Migration: config.MigrationConfig{Enabled: true, Features: []string{"git-upload-pack"}},
-			},
-			environment: map[string]string{
-				"SSH_CONNECTION":       "1",
-				"SSH_ORIGINAL_COMMAND": "git-upload-pack",
-			},
-			arguments:    []string{},
+			desc:         "it returns an UploadPack command",
+			executable:   gitlabShellExec,
+			environment:  buildEnv("git-upload-pack"),
 			expectedType: &uploadpack.Command{},
 		},
 		{
-			desc:       "it returns an UploadArchive command if the feature is enabled",
-			executable: &executable.Executable{Name: executable.GitlabShell},
-			config: &config.Config{
-				GitlabUrl: "http+unix://gitlab.socket",
-				Migration: config.MigrationConfig{Enabled: true, Features: []string{"git-upload-archive"}},
-			},
-			environment: map[string]string{
-				"SSH_CONNECTION":       "1",
-				"SSH_ORIGINAL_COMMAND": "git-upload-archive",
-			},
-			arguments:    []string{},
+			desc:         "it returns an UploadArchive command",
+			executable:   gitlabShellExec,
+			environment:  buildEnv("git-upload-archive"),
 			expectedType: &uploadarchive.Command{},
 		},
 		{
-			desc:       "it returns a AuthorizedKeys command if the feature is enabled",
-			executable: &executable.Executable{Name: executable.AuthorizedKeysCheck},
-			config: &config.Config{
-				Migration: config.MigrationConfig{Enabled: true, Features: []string{"gitlab-shell-authorized-keys-check"}},
-			},
-			environment:  map[string]string{},
+			desc:         "it returns a AuthorizedKeys command",
+			executable:   authorizedKeysExec,
 			arguments:    []string{"git", "git", "key"},
 			expectedType: &authorizedkeys.Command{},
 		},
 		{
-			desc:       "it returns a AuthorizedPrincipals command if the feature is enabled",
-			executable: &executable.Executable{Name: executable.AuthorizedPrincipalsCheck},
-			config: &config.Config{
-				Migration: config.MigrationConfig{Enabled: true, Features: []string{"gitlab-shell-authorized-principals-check"}},
-			},
-			environment:  map[string]string{},
+			desc:         "it returns a AuthorizedPrincipals command",
+			executable:   authorizedPrincipalsExec,
 			arguments:    []string{"key", "principal"},
 			expectedType: &authorizedprincipals.Command{},
 		},
-		{
-			desc:       "it returns a Fallback command if the feature is unimplemented",
-			executable: &executable.Executable{Name: executable.GitlabShell},
-			config: &config.Config{
-				GitlabUrl: "http+unix://gitlab.socket",
-				Migration: config.MigrationConfig{Enabled: true, Features: []string{"git-unimplemented-feature"}},
-			},
-			environment: map[string]string{
-				"SSH_CONNECTION":       "1",
-				"SSH_ORIGINAL_COMMAND": "git-unimplemented-feature",
-			},
-			arguments:    []string{},
-			expectedType: &fallback.Command{},
-		},
-		{
-			desc:         "it returns a Fallback command if executable is unknown",
-			executable:   &executable.Executable{Name: "unknown"},
-			config:       &config.Config{},
-			arguments:    []string{},
-			expectedType: &fallback.Command{},
-		},
 	}
 
 	for _, tc := range testCases {
@@ -174,7 +98,7 @@ func TestNew(t *testing.T) {
 			restoreEnv := testhelper.TempEnv(tc.environment)
 			defer restoreEnv()
 
-			command, err := New(tc.executable, tc.arguments, tc.config, nil)
+			command, err := New(tc.executable, tc.arguments, basicConfig, nil)
 
 			require.NoError(t, err)
 			require.IsType(t, tc.expectedType, command)
@@ -183,9 +107,33 @@ func TestNew(t *testing.T) {
 }
 
 func TestFailingNew(t *testing.T) {
-	t.Run("It returns an error parsing arguments failed", func(t *testing.T) {
-		_, err := New(&executable.Executable{Name: executable.GitlabShell}, []string{}, &config.Config{}, nil)
+	testCases := []struct {
+		desc          string
+		executable    *executable.Executable
+		environment   map[string]string
+		expectedError error
+	}{
+		{
+			desc:          "Parsing environment failed",
+			executable:    gitlabShellExec,
+			expectedError: errors.New("Only SSH allowed"),
+		},
+		{
+			desc:          "Unknown command given",
+			executable:    gitlabShellExec,
+			environment:   buildEnv("unknown"),
+			expectedError: disallowedcommand.Error,
+		},
+	}
 
-		require.Error(t, err)
-	})
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			restoreEnv := testhelper.TempEnv(tc.environment)
+			defer restoreEnv()
+
+			command, err := New(tc.executable, []string{}, basicConfig, nil)
+			require.Nil(t, command)
+			require.Equal(t, tc.expectedError, err)
+		})
+	}
 }

go/internal/command/fallback/fallback.go

-package fallback
-
-import (
-	"errors"
-	"fmt"
-	"os"
-	"path/filepath"
-	"syscall"
-
-	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/commandargs"
-	"gitlab.com/gitlab-org/gitlab-shell/go/internal/executable"
-)
-
-type Command struct {
-	Executable *executable.Executable
-	RootDir    string
-	Args       commandargs.CommandArgs
-}
-
-var (
-	// execFunc is overridden in tests
-	execFunc  = syscall.Exec
-	whitelist = []string{
-		executable.GitlabShell,
-		executable.AuthorizedKeysCheck,
-		executable.AuthorizedPrincipalsCheck,
-	}
-)
-
-func (c *Command) Execute() error {
-	if !c.isWhitelisted() {
-		return errors.New("Failed to execute unknown executable")
-	}
-
-	rubyCmd := c.fallbackProgram()
-
-	// Ensure rubyArgs[0] is the full path to gitlab-shell-ruby
-	rubyArgs := append([]string{rubyCmd}, c.Args.GetArguments()...)
-
-	return execFunc(rubyCmd, rubyArgs, os.Environ())
-}
-
-func (c *Command) isWhitelisted() bool {
-	for _, item := range whitelist {
-		if c.Executable.Name == item {
-			return true
-		}
-	}
-
-	return false
-}
-
-func (c *Command) fallbackProgram() string {
-	fileName := fmt.Sprintf("%s-ruby", c.Executable.Name)
-
-	return filepath.Join(c.RootDir, "bin", fileName)
-}

go/internal/command/fallback/fallback_test.go

-package fallback
-
-import (
-	"errors"
-	"os"
-	"testing"
-
-	"github.com/stretchr/testify/require"
-
-	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/commandargs"
-	"gitlab.com/gitlab-org/gitlab-shell/go/internal/executable"
-)
-
-type fakeExec struct {
-	OldExec func(string, []string, []string) error
-	Error   error
-	Called  bool
-
-	Filename string
-	Args     []string
-	Env      []string
-}
-
-var (
-	fakeArgs = &commandargs.GenericArgs{Arguments: []string{"foo", "bar"}}
-)
-
-func (f *fakeExec) Exec(filename string, args []string, env []string) error {
-	f.Called = true
-
-	f.Filename = filename
-	f.Args = args
-	f.Env = env
-
-	return f.Error
-}
-
-func (f *fakeExec) Setup() {
-	f.OldExec = execFunc
-	execFunc = f.Exec
-}
-
-func (f *fakeExec) Cleanup() {
-	execFunc = f.OldExec
-}
-
-func TestExecuteExecsCommandSuccesfully(t *testing.T) {
-	cmd := &Command{Executable: &executable.Executable{Name: executable.GitlabShell}, RootDir: "/tmp", Args: fakeArgs}
-
-	// Override the exec func
-	fake := &fakeExec{}
-	fake.Setup()
-	defer fake.Cleanup()
-
-	require.NoError(t, cmd.Execute())
-	require.True(t, fake.Called)
-	require.Equal(t, fake.Filename, "/tmp/bin/gitlab-shell-ruby")
-	require.Equal(t, fake.Args, []string{"/tmp/bin/gitlab-shell-ruby", "foo", "bar"})
-	require.Equal(t, fake.Env, os.Environ())
-}
-
-func TestExecuteExecsUnknownExecutable(t *testing.T) {
-	cmd := &Command{Executable: &executable.Executable{Name: "unknown"}, RootDir: "/test"}
-
-	require.Error(t, cmd.Execute())
-}
-
-func TestExecuteExecsCommandOnError(t *testing.T) {
-	cmd := &Command{Executable: &executable.Executable{Name: executable.GitlabShell}, RootDir: "/test", Args: fakeArgs}
-
-	// Override the exec func
-	fake := &fakeExec{Error: errors.New("Test error")}
-	fake.Setup()
-	defer fake.Cleanup()
-
-	require.Error(t, cmd.Execute())
-	require.True(t, fake.Called)
-}
-
-func TestExecuteGivenNonexistentCommand(t *testing.T) {
-	cmd := &Command{Executable: &executable.Executable{Name: executable.GitlabShell}, RootDir: "/tmp/does/not/exist", Args: fakeArgs}
-
-	require.Error(t, cmd.Execute())
-}

go/internal/config/config.go

@@ -16,11 +16,6 @@ const (
 	defaultSecretFileName = ".gitlab_shell_secret"
 )
 
-type MigrationConfig struct {
-	Enabled  bool     `yaml:"enabled"`
-	Features []string `yaml:"features"`
-}
-
 type HttpSettingsConfig struct {
 	User               string `yaml:"user"`
 	Password           string `yaml:"password"`
@@ -34,7 +29,6 @@ type Config struct {
 	RootDir        string
 	LogFile        string             `yaml:"log_file"`
 	LogFormat      string             `yaml:"log_format"`
-	Migration      MigrationConfig    `yaml:"migration"`
 	GitlabUrl      string             `yaml:"gitlab_url"`
 	GitlabTracing  string             `yaml:"gitlab_tracing"`
 	SecretFilePath string             `yaml:"secret_file"`
@@ -56,20 +50,6 @@ func NewFromDir(dir string) (*Config, error) {
 	return newFromFile(path.Join(dir, configFile))
 }
 
-func (c *Config) FeatureEnabled(featureName string) bool {
-	if !c.Migration.Enabled {
-		return false
-	}
-
-	for _, enabledFeature := range c.Migration.Features {
-		if enabledFeature == featureName {
-			return true
-		}
-	}
-
-	return false
-}
-
 func newFromFile(filename string) (*Config, error) {
 	cfg := &Config{RootDir: path.Dir(filename)}
 

go/internal/config/config_test.go

@@ -28,7 +28,6 @@ func TestParseConfig(t *testing.T) {
 		path         string
 		format       string
 		gitlabUrl    string
-		migration    MigrationConfig
 		secret       string
 		httpSettings HttpSettingsConfig
 	}{
@@ -55,13 +54,6 @@ func TestParseConfig(t *testing.T) {
 			format: "json",
 			secret: "default-secret-content",
 		},
-		{
-			yaml:      "migration:\n  enabled: true\n  features:\n    - foo\n    - bar",
-			path:      path.Join(testRoot, "gitlab-shell.log"),
-			format:    "text",
-			migration: MigrationConfig{Enabled: true, Features: []string{"foo", "bar"}},
-			secret:    "default-secret-content",
-		},
 		{
 			yaml:      "gitlab_url: http+unix://%2Fpath%2Fto%2Fgitlab%2Fgitlab.socket",
 			path:      path.Join(testRoot, "gitlab-shell.log"),
@@ -110,8 +102,6 @@ func TestParseConfig(t *testing.T) {
 			err := parseConfig([]byte(tc.yaml), &cfg)
 			require.NoError(t, err)
 
-			assert.Equal(t, tc.migration.Enabled, cfg.Migration.Enabled, "migration.enabled not equal")
-			assert.Equal(t, tc.migration.Features, cfg.Migration.Features, "migration.features not equal")
 			assert.Equal(t, tc.path, cfg.LogFile)
 			assert.Equal(t, tc.format, cfg.LogFormat)
 			assert.Equal(t, tc.gitlabUrl, cfg.GitlabUrl)
@@ -120,64 +110,3 @@ func TestParseConfig(t *testing.T) {
 		})
 	}
 }
-
-func TestFeatureEnabled(t *testing.T) {
-	testCases := []struct {
-		desc          string
-		config        *Config
-		feature       string
-		expectEnabled bool
-	}{
-		{
-			desc: "When the protocol is supported and the feature enabled",
-			config: &Config{
-				GitlabUrl: "http+unix://gitlab.socket",
-				Migration: MigrationConfig{Enabled: true, Features: []string{"discover"}},
-			},
-			feature:       "discover",
-			expectEnabled: true,
-		},
-		{
-			desc: "When the protocol is supported and the feature is not enabled",
-			config: &Config{
-				GitlabUrl: "http+unix://gitlab.socket",
-				Migration: MigrationConfig{Enabled: true, Features: []string{}},
-			},
-			feature:       "discover",
-			expectEnabled: false,
-		},
-		{
-			desc: "When the protocol is supported and all features are disabled",
-			config: &Config{
-				GitlabUrl: "http+unix://gitlab.socket",
-				Migration: MigrationConfig{Enabled: false, Features: []string{"discover"}},
-			},
-			feature:       "discover",
-			expectEnabled: false,
-		},
-		{
-			desc: "When the protocol is http and the feature enabled",
-			config: &Config{
-				GitlabUrl: "http://localhost:3000",
-				Migration: MigrationConfig{Enabled: true, Features: []string{"discover"}},
-			},
-			feature:       "discover",
-			expectEnabled: true,
-		},
-		{
-			desc: "When the protocol is https and the feature enabled",
-			config: &Config{
-				GitlabUrl: "https://localhost:3000",
-				Migration: MigrationConfig{Enabled: true, Features: []string{"discover"}},
-			},
-			feature:       "discover",
-			expectEnabled: true,
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.desc, func(t *testing.T) {
-			assert.Equal(t, tc.expectEnabled, tc.config.FeatureEnabled(string(tc.feature)))
-		})
-	}
-}