[Freeipa-devel] [PATCH 0074] Make token window sizes configurable

[Petr Vobornik]

On 2.12.2014 20:57, Nathaniel McCallum wrote:
>>
>> I'm little confused with a state of reviews. Thierry were some of the
>> patches ACKed in different threads or are they under review (I'm not
>> reviewing DS plugin parts)?
>
> Patches 0001, 0002, 0003 are ACKed by Thierry, but not merged. They can
> and should be merged as they fix an independent bug.
>
>>>>> Ah, I meant adding the token config to cn=otp,SUFFIX directly, but if we want
>>>>> to make TOTP/HOTP token config as separate entries (to enable future per-token
>>>>> overrides), your approach should make sense. Rather adding Rob to CC for sanity.
>>>>
>>>> That would work too. I'm open to that.
>>>>
>>>>> I am just not sure we should create them as separate plugins, I think the new
>>>>> commands should be rather added to otp plugin directly so that they show in
>>>>> "ipa help otptoken" instead of adding 2 new topics just for OTP config.
>>>>
>>>> I can play with that.
>>
>> Do you plan to change it? I like the idea of a single point of help for
>> OTP but I'm also unsure about the length of the commands. Current
>> solution is also more consistent with a rest of the framework. Would it
>> be something like:
>>
>>     otptoken-totpconfig-(show|mod)
>>     otptoken-hotpconfig-(show|mod)
>
> In the latest patch, I merged totpconfig-* and hotpconfig-* into a
> single otpconfig-* plugin.
>
>> Maybe it would be better to introduce more help topics for otp. This
>> concept is used for HBAC already:
>>
>>     $ ipa help hbac
>>       hbacsvcgroup  HBAC Service Groups
>>       hbacsvc       HBAC Services
>>       hbacrule      Host-based access control
>>
>>     $ ipa help hbacrule
>>     Host-based access control
>>     ... a lot of text
>>
>> So we could introduce otp umbrella topic:
>>
>>     $ ipa help otp
>>       opttoken     OTP tokens'
>>       totpconfig   TOTP configuration options
>>       hotpconfig   HOTP configuration options
>
> I added a fifth patch (0005) which creates an otp umbrella topic. We can
> merge it or not.
>
>>>> Nathaniel
>>>
>>> No worries ATM, you can wait for proper review. I was just looking at the new
>>> API to make sure we are on the same page - we seem to mostly are.
>>>
>>> Martin
>>>
>>
>> Commenting just patch 0004:
>>
>> 1. Requires rebase because of API change.
>
> Fixed.
>
>> 2. git diff HEAD~4 -U0 | pep8 --diff
>> I would ignore E124 and fix E302 (5x)
>
> Fixed.
>
>> I did not test actual functionality yet.
>
> The attached patches I think have a much better overall aesthetic. Now
> patch 0004 introduces only two new commands:
> * otpconfig-mod
> * otpconfig-show
>
> Under the covers, a single configuration entity is used:
> * cn=otp,cn=etc,$SUFFIX
>
> Other than these small changes, there are no changes to patch 0004. I
> have not tested the latest changes, however, due to an unrelated build
> issue I'm working on.
>
> Patch 0005 introduces an umbrella help topic for all OTP related
> commands (currently: otpconfig, otptoken, otptoken-yubikey).
>
> Nathaniel
>

Works fine.

python part of 0004: ACK, but VERSION needs to be updated before push
0005: ACK

One question before push: For per-token configuration, do you intent to 
extend each token, regardless of type, by 'ipatokenOTPConfig' object 
class? I.e. to have config attributes for both types? Or do you plan to 
have special object classes for each token type as we now have for tokens?
-- 
Petr Vobornik