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

[Luc de Louw]

On 04/21/2015 01:08 PM, Jan Cholasta wrote:

The param should be a Flag then.


Okay, will work on that on the week end then.

Thanks,

Luc





--
Luc de Louw
Senior Linux Consultant
Red Hat GmbH
Am Treptower Park 75, 2nd floor
D-12435 Berlin

Email: ldel...@redhat.com
Cell Germany: +49 162 413 29 64

Red Hat GmbH, http://www.de.redhat.com/ Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Michael Cunningham, Michael 
O'Neill, Charles Peters


--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Jan Cholasta]

Dne 17.4.2015 v 13:58 Nathaniel McCallum napsal(a):

On Thu, 2015-04-16 at 09:12 +0200, Jan Cholasta wrote:

Dne 9.4.2015 v 15:11 Luc de Louw napsal(a):


On 04/09/2015 02:28 PM, Jan Cholasta wrote:

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.



I actually started with a bool, default=True. I had the problem
that the
Default value was ignored, the value was None.

Changing the default behavior is IMHO bad anyway does not matter
if Bool
or Flag.


+1



Please advise what is you wish to be implemented :-)


That depends. Is there a difference between "do not set APPEND_CR
ticket
flag" and "set APPEND_CR ticket flag to false"?


For YubiKey hardware the flag is either present (true) or absent
(false). This flag controls whether or not the carriage return is sent
(present) or not (absent).


The param should be a Flag then.

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Nathaniel McCallum]

On Thu, 2015-04-16 at 09:12 +0200, Jan Cholasta wrote:
> Dne 9.4.2015 v 15:11 Luc de Louw napsal(a):
> > 
> > On 04/09/2015 02:28 PM, Jan Cholasta wrote:
> > > > > > 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.
> > > 
> > 
> > I actually started with a bool, default=True. I had the problem 
> > that the
> > Default value was ignored, the value was None.
> > 
> > Changing the default behavior is IMHO bad anyway does not matter 
> > if Bool
> > or Flag.
> 
> +1
> 
> > 
> > Please advise what is you wish to be implemented :-)
> 
> That depends. Is there a difference between "do not set APPEND_CR 
> ticket 
> flag" and "set APPEND_CR ticket flag to false"?

For YubiKey hardware the flag is either present (true) or absent 
(false). This flag controls whether or not the carriage return is sent 
(present) or not (absent).

Nathaniel

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Jan Cholasta]

Dne 9.4.2015 v 15:11 Luc de Louw napsal(a):


On 04/09/2015 02:28 PM, Jan Cholasta wrote:

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.



I actually started with a bool, default=True. I had the problem that the
Default value was ignored, the value was None.

Changing the default behavior is IMHO bad anyway does not matter if Bool
or Flag.


+1



Please advise what is you wish to be implemented :-)


That depends. Is there a difference between "do not set APPEND_CR ticket 
flag" and "set APPEND_CR ticket flag to false"?




Thanks,

Luc



--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Luc de Louw]

On 04/09/2015 02:28 PM, Jan Cholasta wrote:

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.



I actually started with a bool, default=True. I had the problem that the 
Default value was ignored, the value was None.


Changing the default behavior is IMHO bad anyway does not matter if Bool 
or Flag.


Please advise what is you wish to be implemented :-)

Thanks,

Luc

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [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

  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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [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
>>> 
>>>  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.

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [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

 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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [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

 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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Luc de Louw]

On 04/08/2015 05:53 PM, 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 
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.



Here we are

Thanks,

Luc


>From 064436ea7d111c89147213b319ccb76b6090a650 Mon Sep 17 00:00:00 2001
From: Luc de Louw 
Date: Wed, 8 Apr 2015 18:01:38 +0200
Subject: [PATCH] Added last change statement

---
 VERSION | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/VERSION b/VERSION
index 5acbdf1caa287d664df00260620809d06f5e5eb1..54c29d99f9ef03ac69e55da09f495082e38207ce 100644
--- a/VERSION
+++ b/VERSION
@@ -91,4 +91,4 @@ IPA_DATA_VERSION=2010061412
 
 IPA_API_VERSION_MAJOR=2
 IPA_API_VERSION_MINOR=117
-# Last change: tbordaz - Add stageuser_add command"
+# Last change: ldelouw - Add no-cr option to otptoken-add-yubikey"
-- 
2.1.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Nathaniel McCallum]

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

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Martin Basti]

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


--
Martin Basti

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Luc de Louw]

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


>From f1a01a7f984e60df6978a506af94ea9e1d8098ea Mon Sep 17 00:00:00 2001
From: Luc de Louw 
Date: Wed, 8 Apr 2015 17:41:33 +0200
Subject: [PATCH] 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 --no-cr option.
---
 API.txt|  3 ++-
 VERSION|  2 +-
 ipalib/plugins/otptoken_yubikey.py | 13 -
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/API.txt b/API.txt
index f747765d7f9c87761fed0277cd59d1bc3fbd57e9..d2c57e48e775dacdfe70fbd1eae6249f1c82e1ad 100644
--- a/API.txt
+++ b/API.txt
@@ -2661,7 +2661,7 @@ output: Output('completed', , None)
 output: Output('failed', , None)
 output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 command: otptoken_add_yubikey
-args: 1,8,1
+args: 1,9,1
 arg: Str('ipatokenuniqueid?', cli_name='id', primary_key=True)
 option: Str('description?', cli_name='desc')
 option: Bool('ipatokendisabled?', cli_name='disabled')
@@ -2669,6 +2669,7 @@ option: DateTime('ipatokennotafter?', cli_name='not_after')
 option: DateTime('ipatokennotbefore?', cli_name='not_before')
 option: IntEnum('ipatokenotpdigits?', autofill=True, cli_name='digits', default=6, values=(6, 8))
 option: Str('ipatokenowner?', cli_name='owner')
+option: Flag('no_cr?', autofill=True, cli_name='no_cr', default=False, required=False)
 option: IntEnum('slot?', cli_name='slot', values=(1, 2))
 option: Str('version?', exclude='webui')
 output: Output('result', None, None)
diff --git a/VERSION b/VERSION
index b584eb4584ea45881e5329a846dae0df7e231844..5acbdf1caa287d664df00260620809d06f5e5eb1 100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=2010061412
 #  #
 
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=116
+IPA_API_VERSION_MINOR=117
 # Last change: tbordaz - Add stageuser_add command"
diff --git a/ipalib/plugins/otptoken_yubikey.py b/ipalib/plugins/otptoken_yubikey.py
index 58fc18308f0cfe407881b5fbb5e653c5afbd0eba..a48a1294a6349a1721601e2100088a0a458c5e84 100644
--- a/ipalib/plugins/otptoken_yubikey.py
+++ b/ipalib/plugins/otptoken_yubikey.py
@@ -17,7 +17,7 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see .
 
-from ipalib import _, Str, IntEnum
+from ipalib import _, Str, IntEnum, Flag
 from ipalib.errors import NotFound
 from ipalib.plugable import Registry
 from ipalib.frontend import Command
@@ -65,6 +65,12 @@ class otptoken_add_yubikey(Command):
 label=_('YubiKey slot'),
 values=(1, 2),
 ),
+   Flag('no_cr?',
+   cli_name='no_cr',
+   label=_('Do not append a CR after sending the OTP (default: false)'),
+	   default=False,
+	   required=False,
+   ),
 ) + tuple(x for x in otptoken.takes_params if x.name in (
 'description',
 'ipatokenowner',
@@ -107,6 +113,11 @@ class otptoken_add_yubikey(Command):
 cfg = yk.init_config()
 cfg.mode_oat

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

[Martin Basti]

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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Luc de Louw]

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 
from ipaplatform.paths import paths
ImportError: No module named paths

Any hints?

The other changes are ready to submit.

Thanks,

Luc

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Martin Basti]

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.

--
Martin Basti

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Nathaniel McCallum]

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

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Luc de Louw]

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.


Thanks,

Luc

--- /usr/lib/python2.7/site-packages/ipalib/plugins/otptoken_yubikey.py.orig	2015-04-07 16:07:41.842573899 +0200
+++ /usr/lib/python2.7/site-packages/ipalib/plugins/otptoken_yubikey.py	2015-04-08 11:50:09.576701774 +0200
@@ -17,7 +17,7 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see .
 
-from ipalib import _, Str, IntEnum
+from ipalib import _, Str, IntEnum, Flag
 from ipalib.errors import NotFound
 from ipalib.plugable import Registry
 from ipalib.frontend import Command
@@ -62,6 +62,13 @@
 label=_('YubiKey slot'),
 values=(1, 2),
 ),
+   Flag('do_not_append_cr?',
+   cli_name='do_not_append_cr',
+   label=_('Do not append a CR after sending the OTP (default: false)'),
+	   doc=_('Do not append a CR after sending the OTP to prevent submitting the password+OTP (default: false)'),
+	   default=False,
+	   required=False,
+   ),
 ) + tuple(x for x in otptoken.takes_params if x.name in (
 'description',
 'ipatokenowner',
@@ -104,6 +111,11 @@
 cfg = yk.init_config()
 cfg.mode_oath_hotp(key, kwargs['ipatokenotpdigits'])
 cfg.extended_flag('SERIAL_API_VISIBLE', True)
+
+	# If the do_not_append_cr flag was not specified, add the parameter APPEND_CR to the config
+	if kwargs.get('do_not_append_cr') is False:
+		cfg.ticket_flag('APPEND_CR', True)
+
 yk.write_config(cfg, slot=kwargs['slot'])
 
 # Filter the options we want to pass.
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code