Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rclone-browser should not prompt for the config password if RCLONE_PASSWORD_COMMAND is set #79

Open
myelsukov opened this issue Feb 20, 2020 · 30 comments
Labels
will be fixed in the next relase fix is already done and will be included in the next release
Milestone

Comments

@myelsukov
Copy link

rclone-browser should not prompt for the config password if RCLONE_PASSWORD_COMMAND environment variable is set. rclone will use that command to get the configuration password (https://rclone.org/docs/#configuration-encryption)

@kapitainsky
Copy link
Owner

thx for reporting. I will fix it in the next release

@kapitainsky kapitainsky added bug Something isn't working will be fixed in the next relase fix is already done and will be included in the next release labels Feb 20, 2020
@myelsukov
Copy link
Author

Also, the prompt is not needed if rclone --password-command parameter is provided in the rclone-browser configuration

@kapitainsky
Copy link
Owner

rclone-browser should not prompt for the config password if RCLONE_PASSWORD_COMMAND environment variable is set. rclone will use that command to get the configuration password (https://rclone.org/docs/#configuration-encryption)

This is how it works at the moment... if RCLONE_PASSWORD_COMMAND is set and working you wont be asked for password. Have you tested it? It works for me perfectly

@kapitainsky kapitainsky added will be fixed in the next relase fix is already done and will be included in the next release and removed bug Something isn't working will be fixed in the next relase fix is already done and will be included in the next release labels Feb 21, 2020
@kapitainsky
Copy link
Owner

kapitainsky commented Feb 21, 2020

Also, the prompt is not needed if rclone --password-command parameter is provided in the rclone-browser configuration

I assume you refer to default rclone option list from preferences. Indeed this has to be rectified. As at the moment list is not used for listing remotes (this is when it is detected if password is needed). I will fix it in the next release.

In the meantime use env variable RCLONE_PASSWORD_COMMAND. It works

@myelsukov
Copy link
Author

This is how it works at the moment... if RCLONE_PASSWORD_COMMAND is set and working you wont be asked for password. Have you tested it? It works for me perfectly

What do you mean by that? I reported the issue because rclone-browser didn't work for me (rclone itself works just fine). Every time I start rclone-browser it would ask me to enter configuration password even though I have RCLONE_PASSWORD_COMMAND environment variable properly set. I looked in your code and found that RCLONE_PASSWORD_COMMAND is not checked. Only RCLONE_CONFIG_PASS variable is checked.

So my proposal was: Don't prompt for the password if either RCLONE_PASSWORD_COMMAND environment variable is set or if '--password-command' parameter is provided in the rclone-browser configuration

@myelsukov
Copy link
Author

OK. I think I know what is happening. I will check it in a couple of hours.
Disclaimer: I think that you were right :)

@kapitainsky
Copy link
Owner

I am not checking any variables - decision to ask for password is made based on rclone working or not. If "rclone listremotes --long --ask-password=false" fails then RB will ask for password.
I have just tested RCLONE_PASSWORD_COMMAND env and works for me without any issues. Are you using the latest rclone in RB? RCLONE_PASSWORD_COMMAND story is only supported by latest one.

@myelsukov
Copy link
Author

myelsukov commented Feb 21, 2020

You are absolutely right. I overlooked that part. In my case the rclone-browser is launched by the Windows Manager which do not have RCLONE_PASSWORD_COMMAND in the environment.

Unfortunately, when I try to overcome it by adding --password-command="pass net/rclone" to the Default rclone options rclone-browser fails to proceed. I believe it's because of "pass net/rclone". Actually there are two problems there: 1. It looks like you are not using Default rclone options when you do your sanity check rclone listremotes --long --ask-password=false and 2. It looks like you are improperly passing "pass net/rclone" argument to rclone.

@kapitainsky
Copy link
Owner

Unfortunately, when I try to overcome it by adding --password-command="pass net/rclone" to the Default rclone options rclone-browser fails to proceed. I believe it's because of "pass net/rclone". Actually there are two problems there: 1. It looks like you are not using Default rclone options when you do your sanity check rclone listremotes --long --ask-password=false and 2. It looks like you are improperly passing "pass net/rclone" argument to rclone.

Correct - this part has to be fixed.

kapitainsky added a commit that referenced this issue Feb 21, 2020
to allow spaces inside option e.g. --password-command="pass rclone/config"
@kapitainsky
Copy link
Owner

The issue is PITA... I thought that I solved this by using regex:

if (!extra.isEmpty()) {
// split on spaces but not if inside quotes e.g. --option-1 --option-2="arg1 arg2" --option-3 arg3
// should generate "--option-1" "--option-2=\"arg1 arg2\"" "--option-3" "arg3"
for (QString arg : extra.split(QRegExp(" (?=[^\"]*(\"[^\"]*\"[^\"]*)*$)"))) {
if (!arg.isEmpty()) {
list << arg;
}
}
}

but it works perfectly on macOS and Linux only. Fails on Windows.

This is beauty of cross platform dev:)

@FoxP
Copy link

FoxP commented Feb 22, 2020

Default OS encoding-related issue maybe?

@kapitainsky
Copy link
Owner

After a bit more time wasted and I discovered what was wrong. My test case among others included option:

--password-command="pass rclone/config"

(this one is really pesky as contains space and quotes). Used regex is supposed to deal with it and it does - on both *nix and windows!! The culprit was rclone version configured on windows. --password-command is only supported by latest version of rclone (v1.51.0)!!! So it was failing because I had previous version of rclone.

I would appreciate if you try to test it. Please find below beta release with the fix. In addition this beta also has some look and feel improvements. Comments welcomed.

https://1drv.ms/u/s!Aq335pidOrBV38Z2UQPBKvFlXIZXXQ

@myelsukov
Copy link
Author

What is Windows? :)

@kapitainsky
Copy link
Owner

kapitainsky commented Feb 22, 2020 via email

@myelsukov
Copy link
Author

I am on Linux and Mac (company issued laptop). I can build Linux version myself but I would appreciate a DMG for Mac.

@kapitainsky
Copy link
Owner

kapitainsky commented Feb 22, 2020 via email

@myelsukov
Copy link
Author

OK. Linux version kind of works. I built the application off kptsky_testing.
It can't parse default rclone options --password command="pass rclone/config" or --password-command "pass rclone/config". It works well with either --password-command get-rclone-config-password or --password-command=get-rclone-config-password

I will test Mac version shortly

@kapitainsky
Copy link
Owner

because --password-command="pass rclone/config" is the only correct one as I understand. I imagine the same would be with rclone cmd.

@kapitainsky
Copy link
Owner

also please note that default rclone options are not used for "rclone mount". There is dedicated prefs field you can use for it.

@myelsukov
Copy link
Author

Sorry, I meant that I put pass rclone/config into the shell script get-rclone-config-password and use it as easier case (no spaces).

So,
rclone listremotes --long --password-command="pass rclone/config" works fine
Default options --password-command get-rclone-config-password works fine
Default options --password-command "pass rclone/config" and --password-command="pass rclone/config" does not work

@kapitainsky
Copy link
Owner

can you put this in for example defaultDownload options instead? then "dry run" some download and from jobs screen you can copy actual rclone command

@myelsukov
Copy link
Author

myelsukov commented Feb 22, 2020

OK. I am trying to test Mac version right now. Need to upgrade rclone first. When I am done with Mac, I'll do that test

@myelsukov
Copy link
Author

myelsukov commented Feb 22, 2020

Unfortunately Mac version doesn't work at all.

rclone listremotes --long --password-command="pass rclone/config" works fine.

I tried default rclone options:

--password-command "pass rclone/config"
--password-command="pass rclone/config"
--password-command=get-rclone-config-password
--password-command get-rclone-config-password
--password-command=/Users/myelsukov/bin/get-rclone-config-password
--password-command /Users/myelsukov/bin/get-rclone-config-password

The get-rclone-config-password shell script dumps environment to the file printenv >~/env.log
It does it when I execute the script manually, but the ~/env.log file is not getting created when I use the script in the default rclone options.

I think you should add some kind of logging to the stderr.

I need to run some errands now. I am not sure if I will be able to find time for testing today.
I hope to test it tomorrow.

Last thing: If I remove --password-command from the default options the rclone-browser prompts me for the password and everything works fine.

@myelsukov
Copy link
Author

Phew!
After some debugging (Linux) I found that this works:

    // --option-1 --option-2="arg1 arg2" --option-3 "arg3 arg4"
    // should generate "--option-1" "--option-2=arg1 arg2" "--option-3" "arg3 arg4"
    for (QString arg : defaultRcloneOptions.split(QRegExp(" (?=[^\"]*(\"[^\"]*\"[^\"]*)*$)"))) {
      if (!arg.isEmpty()) {
        defaultRcloneOptionsList << arg.replace("\"", "");
      }

Normally quotes are processed and removed by the shell. So rclone got the value of the --password-command parameter as "pass rclone/config" obviously, it could not find an executable "pass rclone/config".

kapitainsky added a commit that referenced this issue Feb 23, 2020
@myelsukov
Copy link
Author

I believe you understand that my fix is "quick and dirty". The proper quoting/unquoting is one of the hardest problems. There can be insane combination of single quotes, double quotes, escaped single and double quotes, etc. So your regular expression is not the one that covers all the possible cases.

At the very least, rclone-browser documentation should be very clear in regard of this problem. You should specify what is covered and what is not.

@kapitainsky
Copy link
Owner

kapitainsky commented Feb 23, 2020 via email

@myelsukov
Copy link
Author

Thank you for your application!

@kapitainsky
Copy link
Owner

kapitainsky commented Feb 23, 2020 via email

@myelsukov
Copy link
Author

Well, if you took the responsibility, then it is yours :)

@myelsukov
Copy link
Author

BTW, if you want smart quotes processing you can use a trick like this and let bash to deal with all that jazz:

bash -c 'exec "$@"' bash the-rest-of-the-desired-command-line

The second bash is just a $0 for the exec "$@" script. You can put anything there.

For example:

bash -c 'exec "$@"' bash /usr/bin/rclone listremotes --long --password-command="pass rclone/config"
or
bash -c 'exec "$@"' bash /usr/local/bin/rclone listremotes --long --password-command 'pass "my rclone/config"'

This would also help with setting proper environment for the process using bash rules.
Mac OS GUI applications are running in the minimal environment that does not include stuff from ~/.bashrc and/or ${BASH_ENV} so I have to use full paths in the rclone default options for the Mac version of the rclone-browser.

Play with it.

@kapitainsky kapitainsky added this to the 2.0.0 milestone Mar 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
will be fixed in the next relase fix is already done and will be included in the next release
Projects
None yet
Development

No branches or pull requests

3 participants