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

Migrate to short-term / refresh tokens #103

Closed
transamerican opened this issue Mar 6, 2022 · 23 comments · Fixed by #104
Closed

Migrate to short-term / refresh tokens #103

transamerican opened this issue Mar 6, 2022 · 23 comments · Fixed by #104

Comments

@transamerican
Copy link

transamerican commented Mar 6, 2022

As of September 2021, new apps created for Dropbox do not offer long-term token support. This blog post has more details, but essentially tokens must have an expiration date, and the app Oauth2 settings do not allow you to select "No expiration" anymore.

This results in the access tokens only lasting for a short amount of time (less than a day) before the git-remote-dropbox client throws an error when you try to interact with it:

Unable to refresh access token without refresh token and app key

Thus, anyone who has begun to using this tool after September 2021 must manually log in and retrieve a new generated access token every time their token expires, which is not ideal. It seems like git-remote-dropbox should either migrate to short-lived access tokens or refresh tokens, depending on what it is compatible with.

@anishathalye
Copy link
Owner

Thank you for pointing out. I checked this at some point after September 2021; I don't remember exactly when it was, maybe November 2021, but long-lived access tokens were still supported at that time. Though I just checked again, and it looks like Dropbox has finally removed support for long-lived access tokens.

I will add support for the new tokens, but this might take me some time to get around to doing. PRs are also welcome.

@mohorko
Copy link

mohorko commented Mar 20, 2022

I second this.

git-remote-dropbox is unusable until this is fixed. Tokens expire after 4h.

Idea for a hacky solution in the mean time:

  1. a python script that updates the access token in the git-remote-dropbox.json file
  2. use windows task scheduler to run the script every 4h

My python is very rusty, but I'll try to copy/paste something together. If somebody with better python skills does this, please post you solution here.

@echuber2
Copy link

echuber2 commented Mar 22, 2022

I think this update shouldn't be that hard to implement. What would be the preferred new workflow though?

  1. User gets the short-lived token and puts it in their config file. After (quickly) using the helper, the refresh token can be automatically obtained and stored in the config. After that the short-lived token will soon become useless but the app can keep using the refresh token. In this case, the user will need to provide the app key and secret as additional setup items in their config.

  2. Or, instead, have the user do an extra manual setup step to get the refresh token themselves, and record only the refresh token in the config file. Edit: This doesn't really save any work because the app key and secret are still needed.

Either way, various new request and error handling will need to be added, but it should be pretty straightforward. I may be able to help.

@echuber2
Copy link

echuber2 commented Mar 22, 2022

I'll note that during the bootstrap step to get the refresh key, where it's necessary for the user to click a link like this (from the blog post mentioned above):

https://www.dropbox.com/oauth2/authorize?client_id=YOUR_CLIENT_ID&redirect_uri=YOUR_REDIRECT_URI&response_type=code&token_access_type=offline

The "client id" here is the same as the app key, and the redirect URI property isn't necessary. After clicking through and authorizing, the user gets a short-lived auth code, and this can be used to get the refresh key. So to bootstrap, the app will need to know the app key and app secret, and then have the user retrieve the first short-lived key one time, but after that it can refresh internally. There's no need for anyone to use the "Generate" button on the app console.

@anishathalye
Copy link
Owner

I think we should switch to using PKCE. git-remote-dropbox can store a refresh token in the config file, and whenever it needs to connect to Dropbox, use dropbox.Dropbox(oauth2_refresh_token=refresh_token, app_key=APP_KEY).

We can actually simplify the setup process with this. Rather than making every user create a new Dropbox app, we can just create a single production app, hard-code its APP_KEY in git-remote-dropbox, and have users log in using OAuth (git-remote-dropbox provides a link, user opens the link and clicks authorize, and copy-pastes the token back into the app). We can also deprecate manually editing a config file and have all the configuration happen through CLI programs.

We can add a git dropbox login and git dropbox logout command to facilitate this.

This will drop multi-account support (I am not sure how many people were using that). If there's demand, we could add it back.

I have started implementing this, but I am not sure how much time I will have to work on it in the next week or so.

@echuber2
Copy link

echuber2 commented Mar 22, 2022

After reviewing the stuff about PKCE, I agree. I wasn't sure about the best practice here but Dropbox recommends doing just as you say.

What's the reason for dropping multi-account support? Couldn't the config file just store a separate refresh key for each account? (Edit: I imagine that Alice and Bob shouldn't be storing their keys in the same local user account on a machine, but I can see someone having both personal and work Dropbox accounts.) I can see why the option to use inline tokens might need to be removed though.

Does the dropbox.Dropbox constructor with these options automatically reuse a short-lived token until it expires, or does it actually get a fresh token through the refresh token every single time?

@anishathalye
Copy link
Owner

No strong reason for dropping multi-account support besides it slightly complicating the command-line interface (git dropbox login would need to take an additional argument for the name of the account). In theory, we could even support inline tokens (just have them be refresh tokens), though that seems a bit janky and I think we should just drop it.

Does the dropbox.Dropbox constructor with these options automatically reuse a short-lived token until it expires, or does it actually get a fresh token through the refresh token every single time?

It gets a fresh token through the refresh token every single time. I think that is the intended use.

@echuber2
Copy link

echuber2 commented Mar 23, 2022

As long as getting a fresh token every time doesn't add unnecessary latency, it seems fine. If people have concurrent uses running, hopefully requesting several keys around the same time won't invalidate the ones that haven't been used yet. My first thought had been to use a lockfile mutex on the config file or something and make sure to refresh the short-lived key if 3 hours have passed, but otherwise to just reuse the short-lived key.

If someone is using inline tokens with a legacy permanent token, maybe it's not bad to leave that feature alone, with the understanding that it's no longer useful for new keys.

@anishathalye
Copy link
Owner

It adds some latency (maybe 200-500ms, I didn't measure carefully; I can measure more carefully before merging in this change / add caching if it's problematic). Concurrent requests seem to work out fine.

@ChausseBenjamin
Copy link

If we're overhauling the authentication system, perhaps using a more secure way to save keys could be good. Currently, the API key (which will then become an APP_KEY) is stored in plain text in a json file. If git-remote-dropbox were to query a shell variable, the key could be setup in the following way:

The users .bash_profile or .profile could contain:

export DROPBOX_APP_KEY="$(pass Dropbox/AppKey)"

And they could use gnu-pass to store his key encrypted by running

pass insert Dropbox/AppKey
mkdir: created directory '/home/master/.password-store/Dropbox'
Enter password for Dropbox/AppKey:

@echuber2
Copy link

echuber2 commented Mar 25, 2022

@ChausseBenjamin Just trying to clarify, but I think the "app key" in this context is just the public unique identifier for the Dropbox app, which will be public information specific to the official git-remote-dropbox app as Anish described it. The app secret, on the other hand, is sensitive, but it will be created dynamically using PKCE. The sensitive information that will be persisted locally will be the "refresh token", and this is what the user might want to secure with pass and GPG or whatever. Some other projects like https://github.com/GitCredentialManager/git-credential-manager also allow this flow.

@anishathalye
Copy link
Owner

I've implemented support for PKCE/refresh tokens in #104. I'd really appreciate it if anyone could give feedback on the docs and help test out the code to make sure everything works.

@mohorko
Copy link

mohorko commented Apr 10, 2022

I just tried it and get a permission error on some files:

Traceback (most recent call last):
File "C:\Users\peter\AppData\Local\Programs\Python\Python310\lib\runpy.py", line 196, in _run_module_as_main
return _run_code(code, main_globals, None,
File "C:\Users\peter\AppData\Local\Programs\Python\Python310\lib\runpy.py", line 86, in run_code
exec(code, run_globals)
File "C:\Users\peter\AppData\Local\Programs\Python\Python310\Scripts\git-dropbox-manage.exe_main
.py", line 7, in
File "C:\Users\peter\AppData\Local\Programs\Python\Python310\lib\site-packages\git_remote_dropbox\cli\manage.py", line 55, in main
show_logins()
File "C:\Users\peter\AppData\Local\Programs\Python\Python310\lib\site-packages\git_remote_dropbox\cli\manage.py", line 148, in show_logins
config = get_config()
File "C:\Users\peter\AppData\Local\Programs\Python\Python310\lib\site-packages\git_remote_dropbox\cli\common.py", line 34, in get_config
return Config(path, create=True)
File "C:\Users\peter\AppData\Local\Programs\Python\Python310\lib\site-packages\git_remote_dropbox\util.py", line 135, in init
self.save()
File "C:\Users\peter\AppData\Local\Programs\Python\Python310\lib\site-packages\git_remote_dropbox\util.py", line 166, in save
atomic_write(contents, self._filename)
File "C:\Users\peter\AppData\Local\Programs\Python\Python310\lib\site-packages\git_remote_dropbox\util.py", line 200, in atomic_write
os.replace(temp_file.name, path)
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\Users\peter\.config\git\tmpn5tslxbh' -> 'C:\Users\peter/.config\git\git-remote-dropbox.json'

anishathalye added a commit that referenced this issue Apr 10, 2022
Thanks to @mohorko for finding this issue [1].

[1]: #103 (comment)
@anishathalye
Copy link
Owner

Thank you! I had not tested on Windows. Fixed in 8c42c86.

(And tested on Windows 10 / Python 3.10.1)

@mohorko
Copy link

mohorko commented Apr 11, 2022

I just tried 3d42d65 and have a new problem with git dropbox login - it only wait a couple of seconds before exiting, which is not enough time to copy the code from dropbox. Sometimes it return a KeyboardInterrupt error, sometime not.

$ git dropbox login
Traceback (most recent call last):
File "C:\Users\peter\AppData\Local\Programs\Python\Python310\lib\runpy.py", line 196, in _run_module_as_main
return _run_code(code, main_globals, None,
File "C:\Users\peter\AppData\Local\Programs\Python\Python310\lib\runpy.py", line 86, in run_code
exec(code, run_globals)
File "C:\Users\peter\AppData\Local\Programs\Python\Python310\Scripts\git-dropbox.exe_main
.py", line 7, in
File "C:\Users\peter\AppData\Local\Programs\Python\Python310\lib\site-packages\git_remote_dropbox\cli\manage.py", line 51, in main
login(args.username)
File "C:\Users\peter\AppData\Local\Programs\Python\Python310\lib\site-packages\git_remote_dropbox\cli\manage.py", line 113, in login
auth_code = input("Enter authorization code: ").strip()
File "C:\Users\peter\AppData\Local\Programs\Python\Python310\lib\encodings\cp1252.py", line 22, in decode
def decode(self, input, final=False):
KeyboardInterrupt
Logging in to Dropbox using OAuth...

  1. Go to: https://www.dropbox.com/oauth2/authorize?response_type=code&client_id=h7d8z1irmz0r7lp&token_access_type=offline&code_challenge=mZP_Z10ThcP19H4_FAKE_FAKE_FAKE_W_fKUbYPHoU&code_challenge_method=S256
  2. Click "Allow" (you might have to log in first)
  3. Copy the authorization code
    Enter authorization code:

EDIT: this problem seems to be specific to MINGW64 terminal that comes installed with git on windows. I tried it with the windows console and it worked flawlessly.

@anishathalye
Copy link
Owner

Thanks for checking that it works with the Windows console. I tested on Windows with Git Bash and it's also working correctly there. We're not doing anything fancy to read this input, just using the Python input() function, so I'd expect it to work reliably everywhere. Not sure why you're having issues with MINGW64. Let me know if you have any workarounds for that terminal, I'm happy to incorporate such changes.

@echuber2
Copy link

Does input() break in the MINGW64 terminal in general? If it's at least working in Git Bash (based on MSYS2, I think), then maybe you could just use Git Bash long enough for the migration and refresh token retrieval to work.

@echuber2
Copy link

As part of the migration, perhaps it would be good to put a copy of the long-lived default key in the top level of the json, next to version and tokens. This way, even someone does git dropbox login and upgrades to a refresh token for default, any older versions of the remote that are installed in other envs will continue to work until the long-lived key is revoked.

@anishathalye
Copy link
Owner

Switching to the refresh token doesn't invalidate the long-lived token, so if users are using that on another machine, it will continue to work.

@echuber2
Copy link

Well, what I mean is, someone may have multiple Python project envs on the same machine with different versions of git-remote-dropbox installed in them. Maybe there's some reason why they can't (or don't want) to upgrade them all at the same time. The migration changes the old json file in such a way that isn't backwards-compatible (even if it retains the long-lived token), so some of those local installs of the app will fail to read the config after the migration. However, it's backwards compatible if the json is formatted with some redundancy like:

{
  "version": 2,
  "tokens": {
    "default": [
      "long-lived",
      "foobarbaz1234567890"
    ],
    "named": {}
  },
  "default": "foobarbaz1234567890"
}

@echuber2
Copy link

Here's another way it could be formatted that's implicitly backwards-compatible:

{
  "__version__": 2,
  "__refresh-tokens__": {
    "default": null,
    "named": {
    	"bob": "laskjdf29734987"
    }
  },
  "default": "foobarbaz1234567890",
  "jim": "woeuroie876349835"
}

Here the top-level keys without underscores are assumed to be long-lived keys as before, while __refresh-tokens__ stores only refresh tokens. This avoids duplication.

@echuber2
Copy link

echuber2 commented Apr 15, 2022

One other alternative:

{
  "__version__": 2,
  "__token-types__": {
    "default": "long-lived",
    "jim": "long-lived",
    "bob": "refresh"
  },
  "default": "foobarbaz1234567890",
  "jim": "woeuroie876349835",
  "bob": "laskjdf29734987"
}

This is probably worse than the previous alternative because in the long run, people will only have refresh tokens, so the __token-types__ dictionary would be bloat.

@anishathalye
Copy link
Owner

anishathalye commented Apr 15, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants