Repository: rclone/rclone
Pull request #4437 (2020-07-14 18:13:55)

core/command: Allow rc to execute rclone terminal commands.
<!--
Thank you very much for contributing code or documentation to rclone! Please
fill out the following questions to make it easier for us to review your
changes.

You do not need to check all the boxes below all at once, feel free to take
your time and add more commits. If you're done and ready for review, please
check the last box.
-->

#### What is the purpose of this change?
This will allow rclone to run the commands which are not available directly through rc, over rc.

This api will spawn a temporary instance of rclone to execute the command. Then it will return the raw command output.
This will also help in the development of a web-based rclone terminal. (
<!--
Describe the changes here
-->

#### Was the change discussed in an issue or in the forum before?

<!--
Link issues and relevant forum posts here.
-->

#### Checklist

- [x] I have read the [contribution guidelines](https://github.com/rclone/rclone/blob/master/CONTRIBUTING.md#submitting-a-pull-request).
- [x] I have added tests for all changes in this PR if appropriate.
- [x] I have added documentation for the changes if appropriate.
- [x] All commit messages are in [house style](https://github.com/rclone/rclone/blob/master/CONTRIBUTING.md#commit-messages).
- [x] I'm done, this Pull Request is ready for review :-)


negative0: @ncw, I think the tests are failing because, in the test environment, the rclone executable might be a test executable. What do you think might be the case?

negative0: Executable: /tmp/go-build332533305/b986/rc.testExecutable: /tmp/go-build332533305/b986/rc.testExecutable: /tmp/go-build332533305/b986/rc.testExecutable: /tmp/go-build332533305/b986/rc.testExecutable: /tmp/go-build332533305/b986/rc.testExecutable: /tmp/go-build332533305/b986/rc.testExecutable: /tmp/go-build332533305/b986/rc.testExecutable: /tmp/go-build332533305/b986/rc.testExecutable: /tmp/go-build332533305/b986/rc.testExecutable: /tmp/go-build332533305/b986/rc.testExecutable: /tmp/go-build332533305/b986/rc.testExecutable: /tmp/go-build332533305/b986/rc.testExecutable: /tmp/go-build332533305/b986/rc.testExecutable: /tmp/go-build332533305/b986/rc.testExecutable

This is the output when printing the value of the executable. It just keeps calling the test executable again and again instead of calling the rclone executable.
negative0: @ncw Do you think we should go something like `go build` and then use that executable?
ncw: I've attempted to fix up the tests here.

The magic is in TestMain which pretends to be `rclone version` if it gets a version parameter.

Crude but effective!

```go
func TestMain(m *testing.M) {
	// Pretend to be rclone version if we have a version string parameter
	if os.Args[len(os.Args)-1] == "version" {
		fmt.Printf("rclone %s\n", fs.Version)
		os.Exit(0)
	}
	os.Exit(m.Run())
}
```
negative0: That's an interesting approach. Thanks for that :-)

ncw: This appears to be failing the tests?

```
--- FAIL: TestCoreCommand (0.00s)
    internal_test.go:149: 
        	Error Trace:	internal_test.go:149
        	Error:      	Received unexpected error:
        	            	response object is required
        	            	expecting http.ResponseWriter value for key "_response" (was *http.ResponseWriter)
        	            	github.com/rclone/rclone/fs/rc.rcRunCommand
        	            		/home/runner/work/rclone/rclone/fs/rc/internal.go:413
        	            	github.com/rclone/rclone/fs/rc.TestCoreCommand
        	            		/home/runner/work/rclone/rclone/fs/rc/internal_test.go:148
        	            	testing.tRunner
        	            		/opt/hostedtoolcache/go/1.14.6/x64/src/testing/testing.go:1039
        	            	runtime.goexit
        	            		/opt/hostedtoolcache/go/1.14.6/x64/src/runtime/asm_amd64.s:1373
        	Test:       	TestCoreCommand
FAIL
```
buengese: What exactly is the use case for this? Every functionality required for the WebUI should be exposed directly via the rc interface. I don't really see why anyone would wanna use a web based rclone terminal over running rclone in the local terminal or via ssh.

For anything other than a web based terminal this looks like an ugly hack.
ncw: > What exactly is the use case for this? Every functionality required for the WebUI should be exposed directly via the rc interface. 

Every thing that rclone provides should be provided over the API.

Some of it is quite hard to use at the moment and the docs and forum all show `rclone xyz` commands.

There is a problem at the moment with global options which we haven't fixed yet (all the options are global)

> I don't really see why anyone would wanna use a web based rclone terminal over running rclone in the local terminal or via ssh.

A reasonable point. It is really a demo of the new plugin system. It isn't much code and I don't think it opens any security vulnerabilities (which was my main concern).

> For anything other than a web based terminal this looks like an ugly hack.

Yes that is what it is for. And it is an ugly hack!

Do you think we should do this a different way?
buengese: > There is a problem at the moment with global options which we haven't fixed yet (all the options are global)
I've noticed this too is this something you are planning to work on soon?

> Do you think we should do this a different way?
I agree that this is not a security issue and I guess it's good enough for now. In the long term I'd kinda prefer a web console to call the relevant rc commands directly (e.g. running rclone sync in the webui means the webui parses the command and directly calls rc/sync). Though this would be a lot of work and could potentially cause problems with feature parity with the normal command line so it's not ideal either.
ncw: > > There is a problem at the moment with global options which we haven't fixed yet (all the options are global)
> 
> I've noticed this too is this something you are planning to work on soon?

I think what we need to do is attach an fs.Config to the context, then write a little shim `fs.GetConfig(ctx)` which attempts to read the config from the `ctx` and if it isn't found return the global ctx.

The `fs.Config` could be created in the rc by passing in an `_options` struct which could take the same format as returned by `options/get`. There is actually more than one global config struct we might want to set, eg the Filters.

I'd quite like to make a MarshalJSON method for each of the config flags so they could take strings too so you could set `_options: { "main": { "ConnectTimeout": "5m" } }` rather than using `30000000000` nano seconds.

However we have a way of serializing the options already - the command line - so if we could pass in a command line string of options and parse that into config that would probably be much easier to use, so pass in `_flags="--contimeout 5m"`. It would need a bit of rejiggling of internals to make that work, but maybe that is something worth a bit of effort - it would also enable the `core/command` API to work in a much cleaner way.

What do you think?
 
> > Do you think we should do this a different way?
> 
> I agree that this is not a security issue and I guess it's good enough for now. In the long term I'd kinda prefer a web console to call the relevant rc commands directly (e.g. running rclone sync in the webui means the webui parses the command and directly calls rc/sync). Though this would be a lot of work and could potentially cause problems with feature parity with the normal command line so it's not ideal either.

I think if we can use the actual command line parser then it would be a great feature (see above).


buengese: Sounds good to me.
> I'd quite like to make a MarshalJSON method for each of the config flags so they could take strings too so you could set _options: { "main": { "ConnectTimeout": "5m" } } rather than using 30000000000 nano seconds.
This sounds like a bit of work but I quite like the idea given that it improves readability of the options quite a lot.

I'd also already thought of reusing thought of reusing the command line parser but hadn't looked into how that would be done. If that's something that can be done relatively easy than it's definitely the best option.

negative0: > Sounds good to me.
> 
> > I'd quite like to make a MarshalJSON method for each of the config flags so they could take strings too so you could set _options: { "main": { "ConnectTimeout": "5m" } } rather than using 30000000000 nano seconds.
> > This sounds like a bit of work but I quite like the idea given that it improves readability of the options quite a lot.
> 
> I'd also already thought of reusing thought of reusing the command line parser but hadn't looked into how that would be done. If that's something that can be done relatively easy than it's definitely the best option.

That would be really nice


Patch:
diff --git a/fs/rc/internal.go b/fs/rc/internal.go
index a813b006e..2c27cda88 100644
--- a/fs/rc/internal.go
+++ b/fs/rc/internal.go
@@ -4,7 +4,9 @@ package rc
 
 import (
 	"context"
+	"net/http"
 	"os"
+	"os/exec"
 	"runtime"
 	"time"
 
@@ -335,3 +337,135 @@ func rcSetBlockProfileRate(ctx context.Context, in Params) (out Params, err erro
 	runtime.SetBlockProfileRate(int(rate))
 	return nil, nil
 }
+
+func init() {
+	Add(Call{
+		Path:          "core/command",
+		AuthRequired:  true,
+		Fn:            rcRunCommand,
+		NeedsRequest:  true,
+		NeedsResponse: true,
+		Title:         "Run a rclone terminal command over rc.",
+		Help: `This takes the following parameters
+
+- command - a string with the command name
+- arg - a list of arguments for the backend command
+- opt - a map of string to string of options
+
+Returns
+
+- result - result from the backend command
+- error	 - set if rclone exits with an error code
+- returnType - one of ("COMBINED_OUTPUT", "STREAM", "STREAM_ONLY_STDOUT". "STREAM_ONLY_STDERR")
+
+For example
+
+    rclone rc core/command command=ls -a mydrive:/ -o max-depth=1
+	rclone rc core/command -a ls -a mydrive:/ -o max-depth=1
+
+Returns
+
+` + "```" + `
+{
+	"error": false,
+	"result": "<Raw command line output>"
+}
+
+OR 
+{
+	"error": true,
+	"result": "<Raw command line output>"
+}
+
+
+`,
+	})
+}
+
+// rcRunCommand runs an rclone command with the given args and flags
+func rcRunCommand(ctx context.Context, in Params) (out Params, err error) {
+
+	command, err := in.GetString("command")
+	if err != nil {
+		command = ""
+	}
+
+	var opt = map[string]string{}
+	err = in.GetStructMissingOK("opt", &opt)
+	if err != nil {
+		return nil, err
+	}
+
+	var arg = []string{}
+	err = in.GetStructMissingOK("arg", &arg)
+	if err != nil {
+		return nil, err
+	}
+
+	returnType, err := in.GetString("returnType")
+	if err != nil {
+		returnType = "COMBINED_OUTPUT"
+	}
+
+	var httpResponse *http.ResponseWriter
+	httpResponse, err = in.GetHTTPResponseWriter()
+	if err != nil {
+		return nil, errors.Errorf("response object is required\n" + err.Error())
+	}
+
+	var allArgs = []string{}
+	if command != "" {
+		// Add the command eg: ls to the args
+		allArgs = append(allArgs, command)
+	}
+	// Add all from arg
+	for _, cur := range arg {
+		allArgs = append(allArgs, cur)
+	}
+
+	// Add flags to args for eg --max-depth 1 comes in as { max-depth 1 }.
+	// Convert it to [ max-depth, 1 ] and append to args list
+	for key, value := range opt {
+		if len(key) == 1 {
+			allArgs = append(allArgs, "-"+key)
+		} else {
+			allArgs = append(allArgs, "--"+key)
+		}
+		allArgs = append(allArgs, value)
+	}
+
+	// Get the path for the current executable which was used to run rclone.
+	ex, err := os.Executable()
+	if err != nil {
+		return nil, err
+	}
+
+	cmd := exec.CommandContext(ctx, ex, allArgs...)
+
+	if returnType == "COMBINED_OUTPUT" {
+		// Run the command and get the output for error and stdout combined.
+
+		out, err := cmd.CombinedOutput()
+
+		if err != nil {
+			return Params{
+				"result": string(out),
+				"error":  true,
+			}, nil
+		}
+		return Params{
+			"result": string(out),
+			"error":  false,
+		}, nil
+	} else if returnType == "STREAM_ONLY_STDOUT" {
+		cmd.Stdout = *httpResponse
+	} else if returnType == "STREAM_ONLY_STDERR" {
+		cmd.Stderr = *httpResponse
+	} else if returnType == "STREAM" {
+		cmd.Stdout = *httpResponse
+		cmd.Stderr = *httpResponse
+	}
+
+	err = cmd.Run()
+	return nil, err
+}
diff --git a/fs/rc/params.go b/fs/rc/params.go
index ae7f932e5..bdd020b1e 100644
--- a/fs/rc/params.go
+++ b/fs/rc/params.go
@@ -91,7 +91,7 @@ func (p Params) Get(key string) (interface{}, error) {
 	return value, nil
 }
 
-// GetHTTPRequest gets a http.Request parameter associated with the request with the key "HttpRequest"
+// GetHTTPRequest gets a http.Request parameter associated with the request with the key "_request"
 //
 // If the parameter isn't found then error will be of type
 // ErrParamNotFound and the returned value will be nil.
@@ -108,6 +108,23 @@ func (p Params) GetHTTPRequest() (*http.Request, error) {
 	return request, nil
 }
 
+// GetHTTPResponseWriter gets a http.ResponseWriter parameter associated with the request with the key "_response"
+//
+// If the parameter isn't found then error will be of type
+// ErrParamNotFound and the returned value will be nil.
+func (p Params) GetHTTPResponseWriter() (*http.ResponseWriter, error) {
+	key := "_response"
+	value, err := p.Get(key)
+	if err != nil {
+		return nil, err
+	}
+	request, ok := value.(*http.ResponseWriter)
+	if !ok {
+		return nil, ErrParamInvalid{errors.Errorf("expecting *http.ResponseWriter value for key %q (was %T)", key, value)}
+	}
+	return request, nil
+}
+
 // GetString gets a string parameter from the input
 //
 // If the parameter isn't found then error will be of type
diff --git a/fs/rc/rcserver/rcserver.go b/fs/rc/rcserver/rcserver.go
index 7f324c21b..e04cd0696 100644
--- a/fs/rc/rcserver/rcserver.go
+++ b/fs/rc/rcserver/rcserver.go
@@ -265,6 +265,10 @@ func (s *Server) handlePost(w http.ResponseWriter, r *http.Request, path string)
 		in["_request"] = r
 	}
 
+	if call.NeedsResponse {
+		in["_response"] = w
+	}
+
 	// Check to see if it is async or not
 	isAsync, err := in.GetBool("_async")
 	if rc.NotErrParamNotFound(err) {
diff --git a/fs/rc/registry.go b/fs/rc/registry.go
index f2f5a1fb8..4cc80541a 100644
--- a/fs/rc/registry.go
+++ b/fs/rc/registry.go
@@ -17,12 +17,13 @@ type Func func(ctx context.Context, in Params) (out Params, err error)
 // Call defines info about a remote control function and is used in
 // the Add function to create new entry points.
 type Call struct {
-	Path         string // path to activate this RC
-	Fn           Func   `json:"-"` // function to call
-	Title        string // help for the function
-	AuthRequired bool   // if set then this call requires authorisation to be set
-	Help         string // multi-line markdown formatted help
-	NeedsRequest bool   // if set then this call will be passed the original request object as _request
+	Path          string // path to activate this RC
+	Fn            Func   `json:"-"` // function to call
+	Title         string // help for the function
+	AuthRequired  bool   // if set then this call requires authorisation to be set
+	Help          string // multi-line markdown formatted help
+	NeedsRequest  bool   // if set then this call will be passed the original request object as _request
+	NeedsResponse bool   // if set then this call will be passed the original response object as _response
 }
 
 // Registry holds the list of all the registered remote control functions


Test patch:
diff --git a/fs/rc/internal_test.go b/fs/rc/internal_test.go
index 118215301..c9e46a537 100644
--- a/fs/rc/internal_test.go
+++ b/fs/rc/internal_test.go
@@ -2,6 +2,10 @@ package rc
 
 import (
 	"context"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"os"
 	"runtime"
 	"testing"
 
@@ -13,6 +17,15 @@ import (
 	"github.com/rclone/rclone/fs/version"
 )
 
+func TestMain(m *testing.M) {
+	// Pretend to be rclone version if we have a version string parameter
+	if os.Args[len(os.Args)-1] == "version" {
+		fmt.Printf("rclone %s\n", fs.Version)
+		os.Exit(0)
+	}
+	os.Exit(m.Run())
+}
+
 func TestInternalNoop(t *testing.T) {
 	call := Calls.Get("rc/noop")
 	assert.NotNil(t, call)
@@ -119,3 +132,22 @@ func TestCoreQuit(t *testing.T) {
 	_, err := call.Fn(context.Background(), in)
 	require.Error(t, err)
 }
+
+// core/command: Runs a raw rclone command
+func TestCoreCommand(t *testing.T) {
+	call := Calls.Get("core/command")
+
+	var httpResponse http.ResponseWriter = httptest.NewRecorder()
+
+	in := Params{
+		"command":   "version",
+		"opt":       map[string]string{},
+		"arg":       []string{},
+		"_response": &httpResponse,
+	}
+	got, err := call.Fn(context.Background(), in)
+	require.NoError(t, err)
+
+	assert.Equal(t, fmt.Sprintf("rclone %s\n", fs.Version), got["result"])
+	assert.Equal(t, false, got["error"])
+}