-
-
Notifications
You must be signed in to change notification settings - Fork 453
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
Added TOTP-like alias generation and included some extra information on CONTRIBUTING.md #996
base: master
Are you sure you want to change the base?
Conversation
Hey, thanks for the PR! It seems to be a nice feature. I'll leave some comments:
|
I'd say |
@acasajus You're right, I'll increase the keyspace, but can we make this customizable when using custom domains? Maybe not in this PR as it will require adding a button to the front-end, too.
@ntnhon That does sound But that can also be misleading because it would not be a |
I'm glad you told me to write some tests because the previous code had some bugs, but now it should be better. I ended up calling this alias generator |
app/api/views/setting.py
Outdated
if alias_generator not in ["word", "uuid", "random_string"]: | ||
return jsonify(error="Invalid alias_generator"), 400 | ||
|
||
if alias_generator == "word": | ||
user.alias_generator = AliasGeneratorEnum.word.value | ||
else: | ||
elif alias_generator == "uuid": | ||
user.alias_generator = AliasGeneratorEnum.uuid.value | ||
else: | ||
user.alias_generator = AliasGeneratorEnum.random_string.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do
try:
user.alias_generator = AliasGeneratorEnum[user.alias_generator].value
except KeyError:
return return jsonify(error="Invalid alias_generator"), 400
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's more future proof since if there are modifications in enum you don't need to update this code.
Funny, I was thinking that it'd be great if we had a dictionary in the enum class for that, to avoid modifying the same code in the future, but I didn't know that we could already do that. Pretty cool. Will update the code soon 😊 |
Co-authored-by: Adrià Casajús <[email protected]>
@acasajus Done! I think. I also worked a bit on the base enum class |
This would be lite. I would love to see this be added. |
Looks good! However I think "random characters" is a better name than "random string", technically all current email generating methods are "random string" methods. Just a small input though. Additionally I think random character alias length should be defined in the environment variables, just like how the path to the words file is. This to easily propagate eventual changes to the alias length across the whole code base. |
Is there any update on when this could be implemented? EDIT: Related issue here #1252 |
@Nautman it seems that there are still some questions that are not answered/addressed yet in the PR. |
@zafuru anything new to this? I am really hoping that this could be merged soon! thanks for your work! |
Still no update to this? |
@@ -1188,7 +1192,10 @@ def generate_email( | |||
if scheme == AliasGeneratorEnum.uuid.value: | |||
name = uuid.uuid4().hex if in_hex else uuid.uuid4().__str__() | |||
random_email = name + "@" + alias_domain | |||
else: | |||
elif scheme == AliasGeneratorEnum.random_string.value: | |||
name = "".join(random.choices(string.digits + string.ascii_lowercase, k=9)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could reuse exisiting util function here
Line 41 in e4d4317
def random_string(length=10, include_digits=False): |
I personally like this feature, hope this can be merged soon. For reference, DuckDuckGo Email Protection currently uses 8 random lowercase letters and numbers for their address generation and it seems to work well for them. |
Agree. It will be good if implemented |
I like having short aliases, but UUIDs are very long, and I prefer using random characters instead of two random words, so I constantly find myself creating custom aliases like:
This pull request adds a mode that allows creating random aliases just like that one, which is very similar to the code we might get when using one-time-passwords.
I also modified
CONTRIBUTING.md
and explained that to copy theexample.env
, we first have to go back to the root folder. I spent a minute trying to find out why I could not copy that file 😅Please do let me know if there is anything else I need to do before this can be merged 😊