jazzband / django-two-factor-auth

Complete Two-Factor Authentication for Django providing the easiest integration into most Django projects.
MIT License
1.65k stars 447 forks source link

make voice token format customizable #186

Closed patroqueeet closed 2 years ago

patroqueeet commented 7 years ago

acc to https://github.com/Bouke/django-two-factor-auth/blob/f308883d7e1e327f06080da254c75a20e5fcc4fb/two_factor/gateways/twilio/views.py#L83 there is always a dot in between. in german language this changes the way the numbers are spoken by the voice: vier(4) becomes vierte which does confuse the user listening. it would be great if the . can be configured or other ways of making the string understood would be used.

Bouke commented 7 years ago

Indeed, that code is written like that so it will change a number "1234" to "1. 2. 3. 4.". The goal was to have the number "spelled out" instead of read like a number. E.g. "1, 2, 3, 4" instead of "one thousand two hundred thirty four". This worked for the languages I tested with (nl, en). I understand that for other languages it might not work. Can you check what a good solution would be for German and possibly for English as well?

patroqueeet commented 7 years ago

hey, I checked your code and even twilio api docu says that ., is interpreted as a natural pause. but when I tested today I listed to the behavior documented above. I would recommend to make the char a setting so that users can either play around and even try - or similar things e.g. and to make this perfect for their language. alternatively twilio recommends the <pause> tag, but I have no experience with that and do not know how and if that works.

Bouke commented 7 years ago

You can see the XML that Twilio is reading in the tests.

<?xml version="1.0" encoding="UTF-8" ?>'
<Response>
  <Say language="en">Your token is 1. 2. 3. 4. 5. 6. Repeat: 1. 2. 3. 4. 5. 6. Good bye.</Say>
</Response>

From the documentation on Pause, I expect the following should work:

<?xml version="1.0" encoding="UTF-8" ?>'
<Response>
  <Say language="en">Your token is 1<Pause/>2<Pause/>3<Pause/>4<Pause/>5<Pause/>6. 
    Repeat: 1<Pause/>2<Pause/>3<Pause/>4<Pause/>5<Pause/>6. Good bye.</Say>
</Response>

Given that a Pause is at least 1 second, this might be a very slow prompt. That's why I would like to get some experience with a solution that would work for all languages, instead of language-specific workarounds.

patroqueeet commented 7 years ago

hey, reasonable and understood. next steps? how can I support?

Bouke commented 7 years ago

In https://github.com/Bouke/django-two-factor-auth/blob/9ea5da23f2b90a32630171b3c48bc33256d9199d/two_factor/gateways/twilio/views.py#L83 you can change '. ' to <Pause/> to test/verify whether the proposed change works for German. Also, the prompt shouldn't be read-out too slowly to the user. If that doesn't work, you could experiment with other ways to introduce a pause between the numbers.

patroqueeet commented 7 years ago

replaced like that:

        return {
            'site_name': get_current_site(self.request).name,

            # Build the prompt. The numbers have to be clearly pronounced,
            # this is by creating a string like "1. 2. 3. 4. 5. 6.", this way
            # Twilio reads the numbers one by one.
            'token': '<Pause/>'.join(self.kwargs['token']) if self.request.method == 'POST' else '',
        }

the number was said fast like that seven hundred fourty four thousand and ... which I understand is not intended. I also tried - but that was outspoken (in german "Bindestrich"). Finally I ventured to use , and that was actually pretty good. But it failed for the last number because its at the end of the sentence, hence followed by a point.

Perfect result was achieved with using , and this translation string:

Ihr Token ist %(token)s ., Ich wiederhole noch einmal: Ihr Token ist %(token)s  ., Vielen Dank und Auf Wiedersehen.

See the ., with surrounding spaces? It makes a good break and is not transformed into voice.

patroqueeet commented 2 years ago

@Bouke like to refresh this issue because I'd love to see 1.12.3 installed on my project :)

claudep commented 2 years ago

@patroqueeet, what about suggesting a pull request?

patroqueeet commented 2 years ago

@patroqueeet, what about suggesting a pull request?

did create one, tried my best. change is simple

claudep commented 2 years ago

Is there a reason you did not use ., as default value as you experimented in a previous comment?

patroqueeet commented 2 years ago

just made it configurable so others are flexible and as is state is kept. I was running a lokal fork with https://github.com/jazzband/django-two-factor-auth/pull/484/commits/4c0bd3bea844d5361358a71bdf795bbd6bbf6d5a change on PROD till now. Hence I based the decision on that...

claudep commented 2 years ago

Fixed in 6e68b7a182 and 2fc4c378ad07,