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

[Jan Cholasta]

Dne 9.4.2015 v 12:42 Martin Kosek napsal(a):
> 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.
>

If you want to hardcode the default into the plugin, there is no benefit 
in using Bool over Flag, because Flag is actually a Bool with hardcoded 
default value.

-- 
Jan Cholasta