[Freeipa-devel] [PATCH] otptoken_yubikey, append CR by default and add a option for not doing so

[Martin Kosek]

On 04/08/2015 06:03 PM, Nathaniel McCallum wrote:
> On Wed, 2015-04-08 at 17:53 +0200, Martin Basti wrote:
>> On 08/04/15 17:46, Luc de Louw wrote:
>>> On 04/08/2015 05:14 PM, Martin Basti wrote:
>>>> On 08/04/15 17:12, Luc de Louw wrote:
>>>>>
>>>>> On 04/08/2015 05:05 PM, Martin Basti wrote:
>>>>>> On 08/04/15 16:55, Nathaniel McCallum wrote:
>>>>>>> On Wed, 2015-04-08 at 11:57 +0200, Luc de Louw wrote:
>>>>>>>> Hi there,
>>>>>>>>
>>>>>>>> At the moment ipa otptoken-add-yubikey does not add the
>>>>>>>> parameter
>>>>>>>> "APPEND_CR". This prevents submit the password+OTP.
>>>>>>>> APPEND_CR is
>>>>>>>> usually
>>>>>>>> very handy, most people use this functionality.
>>>>>>>>
>>>>>>>> The patch changes the behavior to set APPEND_CR by
>>>>>>>> default and let
>>>>>>>> the
>>>>>>>> user override this by using the the --do-not-append-cr
>>>>>>>> option.
>>>>>>> This patch is very helpful and I would like to see it
>>>>>>> merged. Thanks
>>>>>>> Luc!
>>>>>>>
>>>>>>> 1. This patch needs to be formatted according to the
>>>>>>> FreeIPA
>>>>>>> formatting. See:
>>>>>>> https://www.freeipa.org/page/Contribute/Patch_Format
>>>>>>>
>>>>>>> 2. The flag should be named "no_cr" instead of
>>>>>>> "do_not_append_cr".
>>>>>>>
>>>>>>> 3. The comment is not necessary since what the code does
>>>>>>> is obvious.
>>>>>>>
>>>>>>> Nathaniel
>>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> 4) this patch changes API, so please run ./makeapi to
>>>>>> regenerate
>>>>>> API.txt
>>>>>> file and add changes into patch + please bum API minor
>>>>>> version in
>>>>>> VERSION file
>>>>>>
>>>>>> thanks.
>>>>>>
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> When running makeaip, I get the following error:
>>>>>    File "/home/luc/freeipa/ipalib/constants.py", line 25, in
>>>>> <module>
>>>>>      from ipaplatform.paths import paths
>>>>> ImportError: No module named paths
>>>>>
>>>>> Any hints?
>>>>>
>>>>> The other changes are ready to submit.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Luc
>>>> You may need to run 'make version-upgrade' or 'make' to prepare
>>>> the
>>>> module.
>>>>
>>>> If it will not work, you can send incomplete patch, I will add
>>>> API
>>>> changes there, just bump VERSION please
>>>>
>>>
>>> Martin,
>>>
>>> Thanks for your hints, seems to work, please have a look at it...
>>>
>>> Thanks,
>>>
>>> Luc
>>>
>>>
>> Thanks,
>>
>> please change the comment too
>>
>> -IPA_API_VERSION_MINOR=116
>> +IPA_API_VERSION_MINOR=117
>>    # Last change: tbordaz - Add stageuser_add command"
>>
>> Otherwise patch looks good, but Nathaniel is the OTP guru, he should
>> say
>> final ack.
>
> I'm also a tough reviewer. :)
>
> 1. Remove the unnecessary code comment.
>
> 2. There appears to be inconsistent indentation in the flag parameter
> specification. It is probably a mix of tabs and spaces.
>
> 3. The git commit comment should contain one short summary line
> without terminating punctuation followed by any necessary explanatory
> paragraphs. You can change this via the "--amend" option to "git
> commit". Try the following:
>
> Enable YubiKey carriage return emission via otptoken-add-yubikey
>
> Before this patch, YubiKeys programmed by IPA would not emit the
> carriage return character at the end of the OTP value. This requires
> the user to press his YubiKey and then (unnecessarily) the Enter or
> Return key. After this patch, the user only needs to press the YubiKey.
>
> Should a user desire to omit the carriage return character, the --no-
> cr option can be specified.
>
> Nathaniel
>

One more note to the API. By my experience, using a Flag for a boolean data 
input has often proved to be a bad call.

Let's say you now introduce --no-cr flag. What if we decide to change the 
default to False? How would you then change the option/API?

It is more flexible IMO to just use something like

--cr=TRUE|FALSE with TRUE being the default

Martin