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

[Martin Kosek]

On 04/09/2015 12:30 PM, Jan Cholasta wrote:
> 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.

That was the point - some clients would send "ct" flag, some "no_cr" and there
would have to be special handling.

>> 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.

I though the idea was to append the CR by default, i.e. --append-cr=TRUE|FALSE
with TRUE being the default.