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

Add support for specifying the config in a file, (partially) add resume token support #14

Merged
merged 2 commits into from Jan 27, 2021

Conversation

mbafford
Copy link
Collaborator

This is a ~90% PR, but I'm not going to have time to work on this for the rest of the week, so figured I'd get it in front of you to consider / comment on.

This adds support for loading settings from a config file using the module https://github.com/phha/click_config_file - it's a pretty decent module - it just loads each command line argument from the specified config file and falls back to normal behavior if no config file or the option is not specified in the config.

I also added support for reading (but not writing) the resume token and using that instead of the password. I couldn't find a good way in Click to make two options mutually exclusive - I wanted token to negate the need for password, but if neither was specified, then prompt for password.

The closest I found to an out-of-the-box solution was this module https://pypi.org/project/click-option-group/ - and some discussions from the project maintainer about how this type of thing was out of scope of the project, which doesn't make much sense to me. Honestly, I got frustrated with trying to implement it and rolled my own that's good enough.

If you have better ideas, please let me know.

Re resume token - I'm not sure:

  • how long the resume token lasts for - I've been using the same one for two days now
  • if it's any more secure than just storing an app password - the API implies it's still a full account access

I'm just thinking Google is less likely to lock an account that polls frequently as suspicious if it uses the token.

I think the program should probably save an updated token after every run, and I don't consider this really complete until that's done.

@ndbeals
Copy link
Owner

ndbeals commented Jan 27, 2021

You are a beast!

Don't worry about the failed check, it's a broken github action.

I'm still catching up on your earlier big PR/proposal (Does the TODO here still apply? or does this PR cover much of it?)

@ndbeals
Copy link
Owner

ndbeals commented Jan 27, 2021

About the resume token, I like the idea of using it, but I'm not sure where exactly to store it. ~/.config/keep-exporter/config.cfg is an option. Or should there be no default path, and if the user wants to use the config file we can assume they are advanced enough to know to point to the file with --config?

The two main ways I see people using this is:

  1. a once-off (or infrequent) run to archive or migrate from keep. In this case less setup is better, the -u /-p options or prompts are meant for this.
  2. In a scheduled fashion. Config files or the environment variables are meant for this use.

@ndbeals
Copy link
Owner

ndbeals commented Jan 27, 2021

The token is not any more or less secure than logging in with the password, it is for reducing the amount of authentications you need to make against Google keep. Your line about google locking the account is correct.

With that being said, updating the login token every time this is run is counter-productive to using the token login, and not possible (with token logins).

The login token is only changed when logged in via password. The token remains the same when you login with a token.

The most practical option is to have the tool fallback to password login if token login fails, and updating the token then. My reservations about that approach is the possibility of it messing up automation/cron jobs with false positives/silent failures.

@ndbeals
Copy link
Owner

ndbeals commented Jan 27, 2021

This has worked nicely in my quick testing, merging it.

@ndbeals ndbeals merged commit e3bce0c into ndbeals:master Jan 27, 2021
@mbafford
Copy link
Collaborator Author

I'm still catching up on your earlier big PR/proposal (Does the TODO here still apply? or does this PR cover much of it?)

I've edited this comment now and marked the ones I think no longer apply and left empty checkboxes for the ones I think do.

@ndbeals
Copy link
Owner

ndbeals commented Jan 27, 2021

Cheers, thanks! My presences today will be diminished, I slipped a rib and can barely move my mousing arm.

I invited you as a collaborator, and the new work branch is token-config. There I'm refactoring exporting/login/cli logic to different files. As well as adding config file writing (for updating the token). And maybe check your email when you get a chance :)

@mbafford
Copy link
Collaborator Author

The token is not any more or less secure than logging in with the password, it is for reducing the amount of authentications you need to make against Google keep. Your line about google locking the account is correct.
The login token is only changed when logged in via password. The token remains the same when you login with a token.

Ok, I didn't verify the token behavior, was assuming it stored some state due to the name given by gkeepapi - "resume token" implies to me more than just log back in.

The most practical option is to have the tool fallback to password login if token login fails, and updating the token then. My reservations about that approach is the possibility of it messing up automation/cron jobs with false positives/silent failures.

That depends on expiration of the token. If it never expires, I'd prefer to only store the token in my config - assuming the token cannot be used for an interactive login attempt elsewhere, it's maybe slightly more secure to keep than the password. While the token does give full access, it's less convenient and less of an attack vector than a username + password sitting beside each other.

Ultimately, I would prefer neither but worry about neither (much) in my context - an environment of full trust and zero external access (hopefully!).

@mbafford
Copy link
Collaborator Author

I slipped a rib and can barely move my mousing arm.

Yikes - I hope that's a quick recovery! Email received and appreciated will reply, but might be a couple of days.

Thanks for taking my stomping on your project so gracefully - you caught the full brunt of my weekend's hacking enthusiasm.

@ndbeals
Copy link
Owner

ndbeals commented Jan 27, 2021

Ok, I didn't verify the token behavior, was assuming it stored some state due to the name given by gkeepapi - "resume token" implies to me more than just log back in.

I haven't rigorously tested it's behaviour either, and yeah that assumption would make sense but it doesn't seem to be the case. Looking a bit more at gkeepapi code, it seems that is the account master token. From the code

This is useful if you'd like to authenticate with the API without providing your username & password. Do note that the master token has full access to your account.

There's some discrepancies because all the examples using the token also use the email, guess this bears further investigation.

That depends on expiration of the token. If it never expires, I'd prefer to only store the token in my config - assuming the token cannot be used for an interactive login attempt elsewhere, it's maybe slightly more secure to keep than the password. While the token does give full access, it's less convenient and less of an attack vector than a username + password sitting beside each other.

It's seemingly the google account master token, but I'm not entirely sure, some contradictions in the gkeepapi code have me questioning. I can't imagine that the master token here could control other things if 2FA and app passwords are used.

IDK if it expires, neither does gkeepapi it looks like. I assume it does expire (at least with a password change), but I guess we'll only find out in time.

Agreed about the token being slightly more secure than user+password. At the end of the day if people want to automate this, users will have to write a secret down somewhere and there's only so much we can do to protect them. The buck passes to them to set file permissions correctly. Putting a warning in the readme would be a good idea though.

I'll also add a env var for the token. I use this in a docker container (which I guess I should release too) and prefer to set secrets using .env files (which will also catch docker secrets usage too).

Yikes - I hope that's a quick recovery! Email received and appreciated will reply, but might be a couple of days.

Thanks, I hope so too. And awesome, no rush.

Thanks for taking my stomping on your project so gracefully - you caught the full brunt of my weekend's hacking enthusiasm.

My pleasure, NGL I was kinda bummed you did most of the work I was planning to do but it's also sweet seeing how others do it and learning, your type annotations were great as I just started using them for this project. I'm grateful for your contributions.

@ndbeals
Copy link
Owner

ndbeals commented Jan 27, 2021

And thanks for the updated TODO, right off the bat I think I know the source of the unicode filename dedupe issues: validate_path_string().

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

Successfully merging this pull request may close these issues.

None yet

2 participants