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

[Jan Cholasta]

Dne 8.4.2015 v 22:52 Martin Kosek napsal(a):
> 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?

You would have to add --cr flag.

>
> It is more flexible IMO to just use something like
>
> --cr=TRUE|FALSE with TRUE being the default

I would say --append-cr=TRUE|FALSE with no default, meaning do not add 
the flag to the config at all.

>
> Martin
>


-- 
Jan Cholasta