Merge pull request #82 from OliveTin/exec-coc-simplication

fmt: fmt

internal/acl/acl.go

@@ -86,7 +86,7 @@ func UserFromContext(ctx context.Context, cfg *config.Config) *AuthenticatedUser
 	buildUserAcls(cfg, ret)
 
 	log.WithFields(log.Fields{
-		"username": ret.Username,
+		"username":  ret.Username,
 		"usergroup": ret.Usergroup,
 	}).Infof("UserFromContext")
 

internal/config/config.go

@@ -15,12 +15,12 @@ type Action struct {
 
 // ActionArgument objects appear on Actions.
 type ActionArgument struct {
-	Name    string
-	Title   string
+	Name        string
+	Title       string
 	Description string
-	Type    string
-	Default string
-	Choices []ActionArgumentChoice
+	Type        string
+	Default     string
+	Choices     []ActionArgumentChoice
 }
 
 // ActionArgumentChoice represents a predefined choice for an argument.

internal/executor/arguments.go

@@ -0,0 +1,121 @@
+package executor
+
+import (
+	config "github.com/OliveTin/OliveTin/internal/config"
+	log "github.com/sirupsen/logrus"
+
+	"errors"
+	"net/url"
+	"regexp"
+	"strings"
+)
+
+var (
+	typecheckRegex = map[string]string{
+		"very_dangerous_raw_string": "",
+		"int":                       "^[\\d]+$",
+		"ascii":                     "^[a-zA-Z0-9]+$",
+		"ascii_identifier":          "^[a-zA-Z0-9\\-\\.\\_]+$",
+		"ascii_sentence":            "^[a-zA-Z0-9 \\,\\.]+$",
+	}
+)
+
+func parseActionArguments(rawShellCommand string, values map[string]string, action *config.Action) (string, error) {
+	log.WithFields(log.Fields{
+		"cmd": rawShellCommand,
+	}).Infof("Before Parse Args")
+
+	r := regexp.MustCompile("{{ *?([a-zA-Z0-9_]+?) *?}}")
+	matches := r.FindAllStringSubmatch(rawShellCommand, -1)
+
+	for _, match := range matches {
+		argValue, argProvided := values[match[1]]
+
+		if !argProvided {
+			log.Infof("%v", values)
+			return "", errors.New("Required arg not provided: " + match[1])
+		}
+
+		err := typecheckActionArgument(match[1], argValue, action)
+
+		if err != nil {
+			return "", err
+		}
+
+		log.WithFields(log.Fields{
+			"name":  match[1],
+			"value": argValue,
+		}).Debugf("Arg assigned")
+
+		rawShellCommand = strings.ReplaceAll(rawShellCommand, match[0], argValue)
+	}
+
+	log.WithFields(log.Fields{
+		"cmd": rawShellCommand,
+	}).Infof("After Parse Args")
+
+	return rawShellCommand, nil
+}
+
+func typecheckActionArgument(name string, value string, action *config.Action) error {
+	arg := action.FindArg(name)
+
+	if arg == nil {
+		return errors.New("Action arg not defined: " + name)
+	}
+
+	if len(arg.Choices) > 0 {
+		return typecheckChoice(value, arg)
+	}
+
+	return TypeSafetyCheck(name, value, arg.Type)
+}
+
+func typecheckChoice(value string, arg *config.ActionArgument) error {
+	for _, choice := range arg.Choices {
+		if value == choice.Value {
+			return nil
+		}
+	}
+
+	return errors.New("argument value is not one of the predefined choices")
+}
+
+// TypeSafetyCheck checks argument values match a specific type. The types are
+// defined in typecheckRegex, and, you guessed it, uses regex to check for allowed
+// characters.
+func TypeSafetyCheck(name string, value string, argumentType string) error {
+	if argumentType == "url" {
+		return typeSafetyCheckUrl(name, value)
+	}
+
+	return typeSafetyCheckRegex(name, value, argumentType)
+}
+
+func typeSafetyCheckRegex(name string, value string, argumentType string) error {
+	pattern, found := typecheckRegex[argumentType]
+
+	if !found {
+		return errors.New("argument type not implemented " + argumentType)
+	}
+
+	matches, _ := regexp.MatchString(pattern, value)
+
+	if !matches {
+		log.WithFields(log.Fields{
+			"name":  name,
+			"value": value,
+			"type":  argumentType,
+		}).Warn("Arg type check safety failure")
+
+		return errors.New("invalid argument, doesn't match " + argumentType)
+	}
+
+	return nil
+}
+
+func typeSafetyCheckUrl(name string, value string) error {
+	_, err := url.ParseRequestURI(value)
+
+	return err
+}

internal/executor/arguments_test.go

@@ -0,0 +1,69 @@
+package executor
+
+import (
+	config "github.com/OliveTin/OliveTin/internal/config"
+
+	"github.com/stretchr/testify/assert"
+	"testing"
+)
+
+func TestSanitizeUnsafe(t *testing.T) {
+	assert.Nil(t, TypeSafetyCheck("", "_zomg_ c:/ haxxor ' bobby tables && rm -rf ", "very_dangerous_raw_string"))
+}
+
+func TestSanitizeUnimplemented(t *testing.T) {
+	err := TypeSafetyCheck("", "I am a happy little argument", "greeting_type")
+
+	assert.NotNil(t, err, "Test an argument type that does not exist")
+}
+
+func TestArgumentNameNumbers(t *testing.T) {
+	a1 := config.Action{
+		Title: "Do some tickles",
+		Shell: "echo 'Tickling {{ person1name }}'",
+		Arguments: []config.ActionArgument{
+			{
+				Name: "person1name",
+				Type: "ascii",
+			},
+		},
+	}
+
+	values := map[string]string{
+		"person1name": "Fred",
+	}
+
+	out, err := parseActionArguments(a1.Shell, values, &a1)
+
+	assert.Equal(t, "echo 'Tickling Fred'", out)
+	assert.Nil(t, err)
+}
+
+func TestArgumentNotProvided(t *testing.T) {
+	a1 := config.Action{
+		Title: "Do some tickles",
+		Shell: "echo 'Tickling {{ personName }}'",
+		Arguments: []config.ActionArgument{
+			{
+				Name: "person",
+				Type: "ascii",
+			},
+		},
+	}
+
+	values := map[string]string{}
+
+	out, err := parseActionArguments(a1.Shell, values, &a1)
+
+	assert.Equal(t, "", out)
+	assert.Equal(t, err.Error(), "Required arg not provided: personName")
+}
+
+func TestTypeSafetyCheckUrl(t *testing.T) {
+	assert.Nil(t, TypeSafetyCheck("test1", "http://google.com", "url"), "Test URL: google.com")
+	assert.Nil(t, TypeSafetyCheck("test2", "http://technowax.net:80?foo=bar", "url"), "Test URL: technowax.net with query arguments")
+	assert.Nil(t, TypeSafetyCheck("test3", "http://localhost:80?foo=bar", "url"), "Test URL: localhost with query arguments")
+	assert.NotNil(t, TypeSafetyCheck("test4", "http://lo  host:80", "url"), "Test a badly formed URL")
+	assert.NotNil(t, TypeSafetyCheck("test5", "12345", "url"), "Test a badly formed URL")
+	assert.NotNil(t, TypeSafetyCheck("test6", "_!23;", "url"), "Test a badly formed URL")
+}

internal/executor/executor.go

@@ -8,23 +8,21 @@ import (
 
 	"bytes"
 	"context"
-	"errors"
 	"os/exec"
-	"net/url"
-	"regexp"
-	"strings"
 	"time"
 )
 
-var (
-	typecheckRegex = map[string]string{
-		"very_dangerous_raw_string": "",
-		"int":                       "^[\\d]+$",
-		"ascii":                     "^[a-zA-Z0-9]+$",
-		"ascii_identifier":          "^[a-zA-Z0-9\\-\\.\\_]+$",
-		"ascii_sentence":            "^[a-zA-Z0-9 \\,\\.]+$",
-	}
-)
+// ExecutionRequest is a request to execute an action. It's passed to an
+// Executor. They're created from the grpcapi.
+type ExecutionRequest struct {
+	ActionName         string
+	Arguments          map[string]string
+	action             *config.Action
+	Cfg                *config.Config
+	AuthenticatedUser  *acl.AuthenticatedUser
+	logEntry           *InternalLogEntry
+	finalParsedCommand string
+}
 
 // InternalLogEntry objects are created by an Executor, and represent the final
 // state of execution (even if the command is not executed). It's designed to be
@@ -45,71 +43,14 @@ type InternalLogEntry struct {
 	ActionIcon  string
 }
 
-// ExecutionRequest is a request to execute an action. It's passed to an
-// Executor. They're created from the grpcapi.
-type ExecutionRequest struct {
-	ActionName         string
-	Arguments          map[string]string
-	action             *config.Action
-	Cfg                *config.Config
-	AuthenticatedUser  *acl.AuthenticatedUser
-	logEntry           *InternalLogEntry
-	finalParsedCommand string
-}
-
-type executorStep interface {
-	Exec(*ExecutionRequest) bool
-}
+type executorStepFunc func(*ExecutionRequest) bool
 
 // Executor represents a helper class for executing commands. It's main method
 // is ExecRequest
 type Executor struct {
 	Logs []InternalLogEntry
 
-	chainOfCommand []executorStep
-}
-
-// DefaultExecutor returns an Executor, with a sensible "chain of command" for
-// executing actions.
-func DefaultExecutor() *Executor {
-	e := Executor{}
-	e.chainOfCommand = []executorStep{
-		stepFindAction{},
-		stepACLCheck{},
-		stepParseArgs{},
-		stepLogStart{},
-		stepExec{},
-		stepLogFinish{},
-	}
-
-	return &e
-}
-
-type stepFindAction struct{}
-
-func (s stepFindAction) Exec(req *ExecutionRequest) bool {
-	actualAction := req.Cfg.FindAction(req.ActionName)
-
-	if actualAction == nil {
-		log.WithFields(log.Fields{
-			"actionName": req.ActionName,
-		}).Warnf("Action not found")
-
-		req.logEntry.Stderr = "Action not found"
-
-		return false
-	}
-
-	req.action = actualAction
-	req.logEntry.ActionIcon = actualAction.Icon
-
-	return true
-}
-
-type stepACLCheck struct{}
-
-func (s stepACLCheck) Exec(req *ExecutionRequest) bool {
-	return acl.IsAllowedExec(req.Cfg, req.AuthenticatedUser, req.action)
+	chainOfCommand []executorStepFunc
 }
 
 // ExecRequest processes an ExecutionRequest
@@ -123,7 +64,7 @@ func (e *Executor) ExecRequest(req *ExecutionRequest) *pb.StartActionResponse {
 	}
 
 	for _, step := range e.chainOfCommand {
-		if !step.Exec(req) {
+		if !step(req) {
 			break
 		}
 	}
@@ -143,34 +84,46 @@ func (e *Executor) ExecRequest(req *ExecutionRequest) *pb.StartActionResponse {
 	}
 }
 
-type stepLogStart struct{}
-
-func (e stepLogStart) Exec(req *ExecutionRequest) bool {
-	log.WithFields(log.Fields{
-		"title":   req.action.Title,
-		"timeout": req.action.Timeout,
-	}).Infof("Action starting")
+// DefaultExecutor returns an Executor, with a sensible "chain of command" for
+// executing actions.
+func DefaultExecutor() *Executor {
+	e := Executor{}
+	e.chainOfCommand = []executorStepFunc{
+		stepFindAction,
+		stepACLCheck,
+		stepParseArgs,
+		stepLogStart,
+		stepExec,
+		stepLogFinish,
+	}
 
-	return true
+	return &e
 }
 
-type stepLogFinish struct{}
+func stepFindAction(req *ExecutionRequest) bool {
+	actualAction := req.Cfg.FindAction(req.ActionName)
 
-func (e stepLogFinish) Exec(req *ExecutionRequest) bool {
-	log.WithFields(log.Fields{
-		"title":    req.action.Title,
-		"stdout":   req.logEntry.Stdout,
-		"stderr":   req.logEntry.Stderr,
-		"timedOut": req.logEntry.TimedOut,
-		"exit":     req.logEntry.ExitCode,
-	}).Infof("Action finished")
+	if actualAction == nil {
+		log.WithFields(log.Fields{
+			"actionName": req.ActionName,
+		}).Warnf("Action not found")
+
+		req.logEntry.Stderr = "Action not found"
+
+		return false
+	}
+
+	req.action = actualAction
+	req.logEntry.ActionIcon = actualAction.Icon
 
 	return true
 }
 
-type stepParseArgs struct{}
+func stepACLCheck(req *ExecutionRequest) bool {
+	return acl.IsAllowedExec(req.Cfg, req.AuthenticatedUser, req.action)
+}
 
-func (e stepParseArgs) Exec(req *ExecutionRequest) bool {
+func stepParseArgs(req *ExecutionRequest) bool {
 	var err error
 
 	req.finalParsedCommand, err = parseActionArguments(req.action.Shell, req.Arguments, req.action)
@@ -186,9 +139,28 @@ func (e stepParseArgs) Exec(req *ExecutionRequest) bool {
 	return true
 }
 
-type stepExec struct{}
+func stepLogStart(req *ExecutionRequest) bool {
+	log.WithFields(log.Fields{
+		"title":   req.action.Title,
+		"timeout": req.action.Timeout,
+	}).Infof("Action starting")
 
-func (e stepExec) Exec(req *ExecutionRequest) bool {
+	return true
+}
+
+func stepLogFinish(req *ExecutionRequest) bool {
+	log.WithFields(log.Fields{
+		"title":    req.action.Title,
+		"stdout":   req.logEntry.Stdout,
+		"stderr":   req.logEntry.Stderr,
+		"timedOut": req.logEntry.TimedOut,
+		"exit":     req.logEntry.ExitCode,
+	}).Infof("Action finished")
+
+	return true
+}
+
+func stepExec(req *ExecutionRequest) bool {
 	ctx, cancel := context.WithTimeout(context.Background(), time.Duration(req.action.Timeout)*time.Second)
 	defer cancel()
 
@@ -215,103 +187,3 @@ func (e stepExec) Exec(req *ExecutionRequest) bool {
 
 	return true
 }
-
-func parseActionArguments(rawShellCommand string, values map[string]string, action *config.Action) (string, error) {
-	log.WithFields(log.Fields{
-		"cmd": rawShellCommand,
-	}).Infof("Before Parse Args")
-
-	r := regexp.MustCompile("{{ *?([a-zA-Z0-9_]+?) *?}}")
-	matches := r.FindAllStringSubmatch(rawShellCommand, -1)
-
-	for _, match := range matches {
-		argValue, argProvided := values[match[1]]
-
-		if !argProvided {
-			log.Infof("%v", values)
-			return "", errors.New("Required arg not provided: " + match[1])
-		}
-
-		err := typecheckActionArgument(match[1], argValue, action)
-
-		if err != nil {
-			return "", err
-		}
-
-		log.WithFields(log.Fields{
-			"name":  match[1],
-			"value": argValue,
-		}).Debugf("Arg assigned")
-
-		rawShellCommand = strings.ReplaceAll(rawShellCommand, match[0], argValue)
-	}
-
-	log.WithFields(log.Fields{
-		"cmd": rawShellCommand,
-	}).Infof("After Parse Args")
-
-	return rawShellCommand, nil
-}
-
-func typecheckActionArgument(name string, value string, action *config.Action) error {
-	arg := action.FindArg(name)
-
-	if arg == nil {
-		return errors.New("Action arg not defined: " + name)
-	}
-
-	if len(arg.Choices) > 0 {
-		return typecheckChoice(value, arg)
-	}
-
-	return TypeSafetyCheck(name, value, arg.Type)
-}
-
-func typecheckChoice(value string, arg *config.ActionArgument) error {
-	for _, choice := range arg.Choices {
-		if value == choice.Value {
-			return nil
-		}
-	}
-
-	return errors.New("argument value is not one of the predefined choices")
-}
-
-// TypeSafetyCheck checks argument values match a specific type. The types are
-// defined in typecheckRegex, and, you guessed it, uses regex to check for allowed
-// characters.
-func TypeSafetyCheck(name string, value string, argumentType string) error {
-	if argumentType == "url" {
-		return typeSafetyCheckUrl(name, value)
-	}
-
-	return typeSafetyCheckRegex(name, value, argumentType)
-}
-
-func typeSafetyCheckRegex(name string, value string, argumentType string) error {
-	pattern, found := typecheckRegex[argumentType]
-
-	if !found {
-		return errors.New("argument type not implemented " + argumentType)
-	}
-
-	matches, _ := regexp.MatchString(pattern, value)
-
-	if !matches {
-		log.WithFields(log.Fields{
-			"name":  name,
-			"value": value,
-			"type":  argumentType,
-		}).Warn("Arg type check safety failure")
-
-		return errors.New("invalid argument, doesn't match " + argumentType)
-	}
-
-	return nil
-}
-
-func typeSafetyCheckUrl(name string, value string) error {
-	_, err := url.ParseRequestURI(value)
-
-	return err
-}

internal/executor/executor_test.go

@@ -8,17 +8,6 @@ import (
 	config "github.com/OliveTin/OliveTin/internal/config"
 )
 
-func TestSanitizeUnsafe(t *testing.T) {
-	assert.Nil(t, TypeSafetyCheck("", "_zomg_ c:/ haxxor ' bobby tables && rm -rf ", "very_dangerous_raw_string"))
-}
-
-func TestSanitizeUnimplemented(t *testing.T) {
-	err := TypeSafetyCheck("", "I am a happy little argument", "greeting_type")
-
-	assert.NotNil(t, err, "Test an argument type that does not exist")
-}
-
-
 func testingExecutor() (*Executor, *config.Config) {
 	e := DefaultExecutor()
 
@@ -119,54 +108,3 @@ func TestArgumentNameSnakeCase(t *testing.T) {
 	assert.Equal(t, "echo 'Tickling Fred'", out)
 	assert.Nil(t, err)
 }
-
-func TestArgumentNameNumbers(t *testing.T) {
-	a1 := config.Action{
-		Title: "Do some tickles",
-		Shell: "echo 'Tickling {{ person1name }}'",
-		Arguments: []config.ActionArgument{
-			{
-				Name: "person1name",
-				Type: "ascii",
-			},
-		},
-	}
-
-	values := map[string]string{
-		"person1name": "Fred",
-	}
-
-	out, err := parseActionArguments(a1.Shell, values, &a1)
-
-	assert.Equal(t, "echo 'Tickling Fred'", out)
-	assert.Nil(t, err)
-}
-
-func TestArgumentNotProvided(t *testing.T) {
-	a1 := config.Action{
-		Title: "Do some tickles",
-		Shell: "echo 'Tickling {{ personName }}'",
-		Arguments: []config.ActionArgument{
-			{
-				Name: "person",
-				Type: "ascii",
-			},
-		},
-	}
-
-	values := map[string]string{}
-
-	out, err := parseActionArguments(a1.Shell, values, &a1)
-
-	assert.Equal(t, "", out)
-	assert.Equal(t, err.Error(), "Required arg not provided: personName")
-}
-
-func TestTypeSafetyCheckUrl(t *testing.T) {
-	assert.Nil(t, TypeSafetyCheck("test1", "http://google.com", "url"), "Test URL: google.com")
-	assert.Nil(t, TypeSafetyCheck("test2", "http://technowax.net:80?foo=bar", "url"), "Test URL: technowax.net with query arguments")
-	assert.Nil(t, TypeSafetyCheck("test3", "http://localhost:80?foo=bar", "url"), "Test URL: localhost with query arguments")
-	assert.NotNil(t, TypeSafetyCheck("test4", "http://lo  host:80", "url"), "Test a badly formed URL")
-	assert.NotNil(t, TypeSafetyCheck("test5", "12345", "url"), "Test a badly formed URL")
-	assert.NotNil(t, TypeSafetyCheck("test6", "_!23;", "url"), "Test a badly formed URL")
-}