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

Added TOTP-like alias generation and included some extra information on CONTRIBUTING.md #996

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

zafuru
Copy link

@zafuru zafuru commented May 17, 2022

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:

abw7iy@domain.tld

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 the example.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 😊

@acasajus
Copy link
Collaborator

Hey, thanks for the PR! It seems to be a nice feature. I'll leave some comments:

  • The keyspace is too short. If we get to a million aliases we will have a 1 in 2k chance of collision. Since we can't use upper and lower case characters, at least make it 9 chars long.
  • Please don't call it totp. It stands for time-based one time password. It's neither time based nor a password. Can we call it short or something like that?
  • Can you add a test to make sure the alias is generated as expected?

@ntnhon
Copy link
Contributor

ntnhon commented May 17, 2022

  • Please don't call it totp. It stands for time-based one time password. It's neither time based nor a password. Can we call it short or something like that?

I'd say short_uuid to give a clearer context

@zafuru
Copy link
Author

zafuru commented May 17, 2022

Hey, thanks for the PR! It seems to be a nice feature. I'll leave some comments:

* The keyspace is too short. If we get to a million aliases we will have a 1 in 2k chance of collision. Since we can't use upper and lower case characters, at least make it 9 chars long.

* Please don't call it totp. It stands for time-based one time password. It's neither time based nor a password. Can we call it `short` or something like that?

* Can you add a test to make sure the alias is generated as expected?

@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.
I know it's not a TOTP, but I'm very bad at naming and that's the first thing that came to mind when I saw word and uuid 😅 and yeah, I'll add a test case.

  • Please don't call it totp. It stands for time-based one time password. It's neither time based nor a password. Can we call it short or something like that?

I'd say short_uuid to give a clearer context

@ntnhon That does sound But that can also be misleading because it would not be a uuid, maybe alphanumeric? slid? How about short_code?

@zafuru
Copy link
Author

zafuru commented May 17, 2022

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 random_string like the suffix generator

app/api/views/setting.py Outdated Show resolved Hide resolved
Comment on lines 65 to 73
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
Copy link
Collaborator

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

Copy link
Collaborator

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.

@zafuru
Copy link
Author

zafuru commented May 26, 2022

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 😊

@zafuru
Copy link
Author

zafuru commented May 26, 2022

@acasajus Done! I think. I also worked a bit on the base enum class

@c0nfigurati0n
Copy link

This would be lite. I would love to see this be added.

@kelszo
Copy link

kelszo commented Aug 11, 2022

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.

@Nautman
Copy link

Nautman commented Oct 16, 2022

Is there any update on when this could be implemented? ☺️

EDIT: Related issue here #1252

@nguyenkims
Copy link
Contributor

@Nautman it seems that there are still some questions that are not answered/addressed yet in the PR.

@leon1995
Copy link

@zafuru anything new to this? I am really hoping that this could be merged soon! thanks for your work!

@Nypheena
Copy link

Nypheena commented May 8, 2023

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))
Copy link
Contributor

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

def random_string(length=10, include_digits=False):

@developStorm
Copy link
Contributor

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.

@ghost
Copy link

ghost commented Nov 6, 2023

Agree. It will be good if implemented

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

Successfully merging this pull request may close these issues.

None yet

10 participants