Add "get_provisioning_uri" utility for HOTP/TOTP.

[tonyseek]

Please review it. Thanks.

[Ayrx]

I personally do not think that this is something we should have in the library since it's a Google Authenticator specific thing (although it's probably pretty widely used at this point) and not actually part of the HOTP / TOTP standard.

[alex]

Jenkins, ok to test

[alex]

I'm not sure where I stand on if we should implement this, but if so, a few notes on this patch right off the bat:

• Tests are currently failing
• This should be done using a utility function shared by other HOTP and TOTP, not a mixin

[tonyseek]

@Ayrx @alex Thank you for your review. I modified it into an utility function instead of mixin class. And the broken tests have been fixed.

I know that the provisioning URI is not a part of the RFC4226/RFC6238. But it is a way to let end users be able to configure two-factor authentication with their mobile devices. Personally I hope it could be included in cryptography or exists as a third-party library, not implemented many times in different applications. ;-)

[reaperhulk]

jenkins, retest this please

[reaperhulk]

@tonyseek do you happen to know of other places where this is used outside of Google Authenticator? I'm not opposed to adding this if it has become a de facto standard.

[tonyseek]

@reaperhulk Following mobile apps have support for provisioning with QR code which included this form of URI:

• Google Authenticator
• Duo Security
• Authy (TOTP only)

And many sites with 2-factor authentication are providing this kind of QR code, such as Google, Dropbox, GitHub, Zoho and Slack.

[reaperhulk]

Given the widespread use of this (at least as encoded in QR codes) this doesn't seem like a bad thing to add. Any other opinions? /cc @alex, @public, @Ayrx (do you still feel it doesn't belong?)

[reaperhulk]

Assuming we decide to pull this in we put documentation separately in restructured text. You'll need to add this to the docs there.

[Ayrx]

I don't feel strongly about not having this so I'm fine with this going in.
:)

I'll do a code review when I get to a computer unless someone gets to it
first.
On Thu, 28 May 2015 at 12:54 am Paul Kehrer notifications@github.com
wrote:

Given the widespread use of this (at least as encoded in QR codes) this
doesn't seem like a bad thing to add. Any other opinions? /cc @alex
https://github.com/alex, @public https://github.com/public, @Ayrx
https://github.com/ayrx (do you still feel it doesn't belong?)

—
Reply to this email directly or view it on GitHub
#1990 (comment).

[Ayrx]

If I'm reading this right you are attempting to determine if the otp object is an instance of HOTP and not TOTP? If so this should be rewritten as isinstance(otp, HOTP) instead.

[Ayrx]

Overall API design feedback: I don't like that the public API is a standalone function. In particular it leads to this very ugly bit of code:

if hotp is otp:
    if counter is None:
        raise RuntimeError('"counter" is required for HOTP')
   parameters.append(('counter', int(counter)))

due to HOTP and TOTP having different requirements for a provisioning URL.

Is there anything particularly bad about each class (HOTP and TOTP), having a generate_url method with different parameters? Common code can be factored out into a private function but the consumers of our library do not have to concern themselves with that.

[reaperhulk]

👍 on @Ayrx's comments. @tonyseek could you update the patch with get_provisioning_uri becoming a method on each class?

[tonyseek]

Hi @Ayrx @reaperhulk @alex ,

I turned the get_provisioning_uri into methods of HOTP and TOTP. It is cleaner without the if hotp is otp. Thank you for your review. 😄

The description part has been moved into the rst docs too.

[reaperhulk]

For files we have a licensing preamble and a specific __future__ import we want: https://cryptography.io/en/latest/development/submitting-patches/#code

[reaperhulk]

We're getting close now, thanks for your work so far :)

[tonyseek]

@reaperhulk Updated :-)

[coveralls]

Coverage remained the same at 100.0% when pulling d4e0a83 on tonyseek:key-uri into 43cd3d2 on pyca:master.

[Ayrx]

@tonyseek Still missing the standard license header.

[Ayrx]

"widely supported", "which are using".

[Ayrx]

Other than the standard license header and the small grammar fixes this LGTM. 👍

[tonyseek]

@Ayrx Fixed~

[reaperhulk]

Our typical model for external links is to put them at the bottom of the page like:

.. _`spec of Google Authenticator`: https://github.com/google/google-authenticator/wiki/Key-Uri-Format

Additionally we strongly prefer not to use shortener links where possible so the canonical URL is preferred.

[reaperhulk]

Other than that final nit this LGTM.

[tonyseek]

@reaperhulk The URL have been unshorten and moved to the page bottom.

[reaperhulk]

Thank you @tonyseek! You're welcome to submit a PR to add yourself to the AUTHORS.rst file if you'd like. If you get a chance it'd also be great if you could submit a separate PR to add an entry to the CHANGELOG.rst for this new feature.

[tonyseek]

@reaperhulk I will submit PR soon. Thank you!