abertschi / postcards

A CLI for the Swiss Postcard Creator :postbox:
https://abertschi.ch/blog/2022/receiving-postcards/
MIT License
36 stars 8 forks source link

Tasks to do #2

Closed abertschi closed 7 years ago

abertschi commented 7 years ago

Ideas to implement/review

PexelsPlugin:

FolderPlugin:

Liblor commented 7 years ago

How do you feel about limiting the line length?

79 as stated in PEP8 or 80 as proposed by Google's style guide

Personally I also like 100 as 80 is sometimes a little cumbersome.

abertschi commented 7 years ago

👍 I personally prefer more than 80 as well. 100 seems fine.

Liblor commented 7 years ago

I'll have a look at the "decryption" function, but maybe you've already fixed it by now :smile:

abertschi commented 7 years ago

No, I havent :) Will post here from now on whenever I tackle a new issue/feature.

abertschi commented 7 years ago

I first thought about a more sophisticated way to encrypt credentials than Vigenère cipher. However, since we only obscure the password, and as long as one has access to the source or the script that executes the source, any encryption mechanism is only obfuscation. Further, Vigenère cipher is simple and easy to implement.

What are your two cents on that?

abertschi commented 7 years ago

Currently, we encrypt both the username and the password with the key. With this approach it's hard to keep track on the accounts in the config file. I suggest we either:

or implement both suggestions

Liblor commented 7 years ago

First of all I wouldn't encrypt the username as you mentioned.

I'm not even sure if we should encrypt the password at all. It depends on what we want to achieve. If we want to secure the user from people peaking over his shoulder to see the password base64 is probably enough. But if uploading ones config file to a third party server should be possible, encrypting the password would be necessary. I'm not that happy with the current approach, because it requires the user to remember a key. A user could conclude that the password is therefore safely encrypted.

Edit: But I totally understand your reasoning and adding modern encryption adds a huge overhead :disappointed:

abertschi commented 7 years ago

First of all I wouldn't encrypt the username as you mentioned.

Let's do that.

I'm not that happy with the current approach, because it requires the user to remember a key

First of all, encrypting credentials is optional. So if a user does not want to encrypt passwords, he doesnt need to.

I would like to add an encryption feature for the third party server usecase you mentioned. I am planning to run the script on a vserver i share with friends. However, since the script which decrypts the password is also stored on the server, encryption only adds to obfuscation. So the problem is more fundamental anyways.

I suggest we add a default password in case no key is specified and perhaps an option to set a custom key.

Liblor commented 7 years ago

I am planning to run the script on a vserver i share with friends.

OK in that user case it's only additional obfuscation. So let's keep it that way :)

I suggest we add a default password in case no key is specified and perhaps an option to set a custom key.

Sounds ok

Regarding the parsing error messages, I think we should use argparse itself. At the moment all flags are listed as optional, we can tell argparse that some are required and even specify mutual exclusions (https://docs.python.org/3/library/argparse.html#mutual-exclusion) Maybe I'll refactor some code tomorrow.

abertschi commented 7 years ago

we can tell argparse that some are required and even specify mutual exclusions

Indeed, that'd be cool.

Regarding the parsing error messages, I think we should use argparse itself.

I agree, however, argparse validation only works for low level validation. I.e. these two validations. Most validations are more highlevel after cli arguments are parsed.

abertschi commented 7 years ago

Maybe I'll refactor some code tomorrow.

@Liblor I gave you collaboration rights to make things a bit more convenient. Please create feature branches from now on (i.e. prefix branch names with feature/ and do pull requests if you want changes to be merged.

abertschi commented 7 years ago

Regarding the parsing error messages, I think we should use argparse itself. At the moment all flags are listed as optional, we can tell argparse that some are required and even specify mutual exclusions (https://docs.python.org/3/library/argparse.html#mutual-exclusion) Maybe I'll refactor some code tomorrow.

I think a default value for --config might even be better. If there is a file, let's say config.json in the current directory, use that file. Otherwise set your custom file via --config