[Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add

[Petr Viktorin]

On 11/13/2014 06:02 PM, Nathaniel McCallum wrote:
> On Thu, 2014-11-13 at 16:57 +0100, Petr Viktorin wrote:
>> On 11/13/2014 04:40 PM, Petr Vobornik wrote:
>>> On 13.11.2014 16:19, Nathaniel McCallum wrote:
>>>> On Thu, 2014-11-13 at 16:13 +0100, Petr Vobornik wrote:
>>>>> On 13.11.2014 16:00, Nathaniel McCallum wrote:
>>>>>> On Thu, 2014-11-13 at 13:17 +0100, Martin Kosek wrote:
>>>>>>> On 11/13/2014 07:53 AM, Nathaniel McCallum wrote:
>>>>>>>> On Thu, 2014-11-06 at 15:33 -0500, Nathaniel McCallum wrote:
>>>>>>>>> This is possible because python-qrcode's output now fits in a
>>>>>>>>> standard
>>>>>>>>> terminal. Also, update ipa-otp-import and otptoken-add-yubikey to
>>>>>>>>> disable QR code output as it doesn't make sense in these contexts.
>>>>>>>>>
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4703
>>>>>>>>
>>>>>>>> I removed the period from the doc string to maintain consistency with
>>>>>>>> the rest of the plugin.
>>>>>>>>
>>>>>>>> Nathaniel
>>>>>>>>
>>>>>>>
>>>>>>> I am not reviewing, just curious. What is the purpose of --qrcode
>>>>>>> option with
>>>>>>> default=True?
>>>>>>>
>>>>>>>         takes_options = LDAPCreate.takes_options + (
>>>>>>> -        Flag('qrcode?', label=_('Display QR code')),
>>>>>>> +        Flag('qrcode', label=_('Display QR code'), default=True),
>>>>>>>         )
>>>>>>>
>>>>>>> How can user turn it off? Both passing no option and passing
>>>>>>> --qrcode will get
>>>>>>> the same result, no? :-)
>>>>>>>
>>>>>>> Shouldn't we instead change --qrcode to --no-qrcode and process
>>>>>>> somehow fill
>>>>>>> qrcode=0 when it's enabled? (To achieve API compatibility with old
>>>>>>> clients).
>>>>>>
>>>>>> I see three options:
>>>>>> 1. Don't let the user turn it off from the command line (he can still
>>>>>> turn it off from the Python API).
>>>>>>
>>>>>> 2. Change it to --no-qrcode (API change)
>>>>>>
>>>>>> 3. Change the type to Bool (API change)
>>>>>>
>>>>>> Like you, I like #2 the best. Attached is an implementation.
>>>>>
>>>>> I like --no-qrcode as well.
>>>>>
>>>>> Should we also keep qrcode as 'no_option' to maintain API compatibility
>>>>> (but not CLI)?
>>>>
>>>> I don't think it is necessary. It only makes sense to specify --qrcode
>>>> in an interactive session.
>>>>
>>>
>>> Makes sense.
>>>
>>> ACK
>>>
>>> Not pushing yet to give time for NACK if anybody doesn't agree with the
>>> API change.
>>
>> Hold on, what is happening here?
>>
>> Aren't all clients since 4.0 sending the qrcode option to the server?
>> We absolutely can not break backwards compatibility with released versions.
>> We also should not break the CLI. Just make it a no-op option, and say
>> it's deprecated in the doc.
>
> As I understand the current behavior, the qrcode option is *not* sent to
> the server by default in any scenario.

Nope, defaults are filled in by the client. (And also on the server if 
they're still missing; it's part of the common validation.)

You can try it out, actually:

$ ipa -vv otptoken-add
ipa: INFO: trying https://vm-175.idm.lab.eng.brq.redhat.com/ipa/json
ipa: INFO: Forwarding 'otptoken_add' to json server 
'https://vm-175.idm.lab.eng.brq.redhat.com/ipa/json'
ipa: INFO: Request: {
     "id": 0,
     "method": "otptoken_add",
     "params": [
         [
             null
         ],
         {
             "all": false,
             "ipatokenhotpcounter": 0,
             "ipatokenotpalgorithm": "sha1",
             "ipatokenotpdigits": 6,
             "ipatokenotpkey": 
"5\ufffdK\ufffd1\u000e\ufffd7,\ufffd_\ufffd\ufffd.0\ufffdM\ufffd\u0016\ufffd", 

             "ipatokentotpclockoffset": 0,
             "ipatokentotptimestep": 30,
             "no_members": false,
             "qrcode": false,
             "raw": false,
             "type": "totp",
             "version": "2.108"
         }
     ]
}
ipa: INFO: Response: {
     "error": null,
     "id": 0,
     "principal": "admin at IDM.LAB.ENG.BRQ.REDHAT.COM",
...




-- 
Petr³